Martin Koegler [off-list ref] writes:
From: Martin Koegler <redacted>
The current delta code produces incorrect pack objects for files > 4GB.
Signed-off-by: Martin Koegler <redacted>
---
diff-delta.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
Just pass any file > 4 GB to the delta-compression [by increasing the delta limits].
As file size, a truncated 32bit value will be encoded, leading to broken pack files.
The patch obviously makes the code better and self consistent in
that "struct delta_index" has src_size as ulong, and this function
takes trg_size as ulong, and it was plain wrong for the code to
assume that "i", which is uint, can receive it safely.
In the longer term we might want to move to size_t or even
uintmax_t, as the ulong on a platform may not be long enough in
order to express the largest file size the platform can have, but
this patch (1) is good even without such a change, and (2) gives a
good foundation to build on if we want such a change on top.
Thanks. Will queue.
quoted hunk
diff --git a/diff-delta.c b/diff-delta.c
index 3797ce6..13e5a01 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -319,7 +319,8 @@ create_delta(const struct delta_index *index,
const void *trg_buf, unsigned long trg_size,
unsigned long *delta_size, unsigned long max_size)
{
- unsigned int i, outpos, outsize, moff, msize, val;
+ unsigned int i, val;
+ unsigned long l, outpos, outsize, moff, msize;
int inscnt;
const unsigned char *ref_data, *ref_top, *data, *top;
unsigned char *out;@@ -336,20 +337,20 @@ create_delta(const struct delta_index *index,
return NULL;
/* store reference buffer size */
- i = index->src_size;
- while (i >= 0x80) {
- out[outpos++] = i | 0x80;
- i >>= 7;
+ l = index->src_size;
+ while (l >= 0x80) {
+ out[outpos++] = l | 0x80;
+ l >>= 7;
}
- out[outpos++] = i;
+ out[outpos++] = l;
/* store target buffer size */
- i = trg_size;
- while (i >= 0x80) {
- out[outpos++] = i | 0x80;
- i >>= 7;
+ l = trg_size;
+ while (l >= 0x80) {
+ out[outpos++] = l | 0x80;
+ l >>= 7;
}
- out[outpos++] = i;
+ out[outpos++] = l;
ref_data = index->src_buf;
ref_top = ref_data + index->src_size;
Hi,
On Mon, 7 Aug 2017, Junio C Hamano wrote:
Martin Koegler [off-list ref] writes:
quoted
From: Martin Koegler <redacted>
The current delta code produces incorrect pack objects for files > 4GB.
Signed-off-by: Martin Koegler <redacted>
---
diff-delta.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
Just pass any file > 4 GB to the delta-compression [by increasing the delta limits].
As file size, a truncated 32bit value will be encoded, leading to broken pack files.
The patch obviously makes the code better and self consistent in
that "struct delta_index" has src_size as ulong, and this function
takes trg_size as ulong, and it was plain wrong for the code to
assume that "i", which is uint, can receive it safely.
In the longer term we might want to move to size_t or even
uintmax_t, as the ulong on a platform may not be long enough in
order to express the largest file size the platform can have, but
this patch (1) is good even without such a change, and (2) gives a
good foundation to build on if we want such a change on top.
Thanks. Will queue.
This is sad. There is no "may not be long enough". We already know a
platform where unsigned long is not long enough, don't we? Why leave this
patch in this intermediate state?
If you want to work on data in memory, then size_t is the appropriate data
type. We already use it elsewhere. Let's use it here, too, without the
intermediate bump from the incorrect `int` to the equally incorrect
`long`.
Ciao,
Johannes
On Mon, Aug 07, 2017 at 09:39:12PM +0200, Johannes Schindelin wrote:
If you want to work on data in memory, then size_t is the appropriate data
type. We already use it elsewhere. Let's use it here, too, without the
intermediate bump from the incorrect `int` to the equally incorrect
`long`.
I disagree with "We already use it elsewhere.". The whole delta code uses "unsigned long" -
look at delta.h. Look at unpack-objects.c. Or cache.h. Or pack-objects.c. Or index-pack.c.
Other possible cases:
git grep "unsigned long" |grep size
So the codebase still suggests, that "unsigned long" is the data type for storing object sizes.
I would be fine with resubmitting a patch using size_t/off_t for the touched parts - changing the whole
core code is a too invasive change for a bug fix.
Regards,
Martin
Hi Martin,
On Tue, 8 Aug 2017, Martin Koegler wrote:
On Mon, Aug 07, 2017 at 09:39:12PM +0200, Johannes Schindelin wrote:
quoted
If you want to work on data in memory, then size_t is the appropriate data
type. We already use it elsewhere. Let's use it here, too, without the
intermediate bump from the incorrect `int` to the equally incorrect
`long`.
I disagree with "We already use it elsewhere.".
By "it" I meant "size_t".
The whole delta code uses "unsigned long" - look at delta.h. Look at
unpack-objects.c. Or cache.h. Or pack-objects.c. Or index-pack.c.
I know that. It is a major bug in the source code.
Other possible cases:
git grep "unsigned long" |grep size
Yes, even more bugs.
So the codebase still suggests, that "unsigned long" is the data type
for storing object sizes.
And it would be wrong, and we know it already. Most importantly, Junio
knows it already.
I would be fine with resubmitting a patch using size_t/off_t for the
touched parts - changing the whole core code is a too invasive change
for a bug fix.
Sorry, my mistake: I never meant to burden you with the invasive change.
I only wanted you to change the `int` to `size_t` right away.
Thanks,
Johannes