# [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 6 messages from 2007-04-05 to 2007-04-06. Participants: Dana How, Junio C Hamano, Nicolas Pitre. Thread: https://gitlist.dev/t/7531 ## Dana How, 2007-04-05 22:24 Subject: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 Message-ID: <56b7f5510704051524p28eafc18mae3131ef13cdabfa@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704051524p28eafc18mae3131ef13cdabfa%40mail.gmail.com ``` --- builtin-pack-objects.c | 2 +- builtin-unpack-objects.c | 2 +- index-pack.c | 2 +- sha1_file.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) -- Dana L. How danahow@gmail.com +1 650 804 5991 cell From 90937b722f391abe1cbc035c29b33f500c389334 Mon Sep 17 00:00:00 2001 From: Dana How Date: Thu, 5 Apr 2007 12:15:22 -0700 Subject: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 --- builtin-pack-objects.c | 2 +- builtin-unpack-objects.c | 2 +- index-pack.c | 2 +- sha1_file.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index b5f9648..50246e1 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -1014,7 +1014,7 @@ static void check_object(struct object_entry *entry) ofs = c & 127; while (c & 128) { ofs += 1; - if (!ofs || ofs & ~(~0UL >> 7)) + if (!ofs || ofs & ~(~0UL >> 1)) die("delta base offset overflow in pack for %s", sha1_to_hex(entry->sha1)); c = buf[used_0++]; diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c index 3956c56..84a6c5c 100644 --- a/builtin-unpack-objects.c +++ b/builtin-unpack-objects.c @@ -209,7 +209,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size, base_offset = c & 127; while (c & 128) { base_offset += 1; - if (!base_offset || base_offset & ~(~0UL >> 7)) + if (!base_offset || base_offset & ~(~0UL >> 1)) die("offset value overflow for delta base object"); pack = fill(1); c = *pack; diff --git a/index-pack.c b/index-pack.c index 3c768fb..02722f7 100644 --- a/index-pack.c +++ b/index-pack.c @@ -249,7 +249,7 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_ base_offset = c & 127; while (c & 128) { base_offset += 1; - if (!base_offset || base_offset & ~(~0UL >> 7)) + if (!base_offset || base_offset & ~(~0UL >> 1)) bad_object(obj->offset, "offset value overflow for delta base object"); p = fill(1); c = *p; diff --git a/sha1_file.c b/sha1_file.c index 9c26038..7082d3e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1149,7 +1149,7 @@ static off_t get_delta_base(struct packed_git *p, base_offset = c & 127; while (c & 128) { base_offset += 1; - if (!base_offset || base_offset & ~(~0UL >> 7)) + if (!base_offset || base_offset & ~(~0UL >> 1)) die("offset value overflow for delta base object"); c = base_info[used++]; base_offset = (base_offset << 7) + (c & 127); -- 1.5.1.rc2.18.g9c88-dirty ``` ## Junio C Hamano, 2007-04-05 22:51 Subject: Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 Message-ID: <7vr6qymo3r.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr6qymo3r.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <56b7f5510704051524p28eafc18mae3131ef13cdabfa@mail.gmail.com> ``` "Dana How" writes: > Subject: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 > > --- > builtin-pack-objects.c | 2 +- > builtin-unpack-objects.c | 2 +- > index-pack.c | 2 +- > sha1_file.c | 2 +- > 4 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c > index b5f9648..50246e1 100644 > --- a/builtin-pack-objects.c > +++ b/builtin-pack-objects.c > @@ -1014,7 +1014,7 @@ static void check_object(struct object_entry *entry) > ofs = c & 127; > while (c & 128) { > ofs += 1; > - if (!ofs || ofs & ~(~0UL >> 7)) > + if (!ofs || ofs & ~(~0UL >> 1)) > die("delta base offset overflow in pack for %s", > sha1_to_hex(entry->sha1)); > c = buf[used_0++]; The line after these context does this: ofs = (ofs << 7) + (c & 127); If you do not check the top 7 bits, wouldn't you miss overflow? ``` ## Dana How, 2007-04-05 23:08 Subject: Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 Message-ID: <56b7f5510704051608g4bf9ffb7t9f94b532c7a0ebdf@mail.gmail.com> URL: https://gitlist.dev/e/56b7f5510704051608g4bf9ffb7t9f94b532c7a0ebdf%40mail.gmail.com In-Reply-To: <7vr6qymo3r.fsf@assigned-by-dhcp.cox.net> ``` On 4/5/07, Junio C Hamano wrote: > "Dana How" writes: > > ofs = c & 127; > > while (c & 128) { > > ofs += 1; > > - if (!ofs || ofs & ~(~0UL >> 7)) > > + if (!ofs || ofs & ~(~0UL >> 1)) > > die("delta base offset overflow in pack for %s", > > sha1_to_hex(entry->sha1)); > > c = buf[used_0++]; > The line after these context does this: > ofs = (ofs << 7) + (c & 127); > If you do not check the top 7 bits, wouldn't you miss overflow? You are correct in all 4 cases -- this patch can be dropped from the set; it has no overlap in any context. Concerning SEEK_SET, you are correct: my unistd.h #define's SEEK_SET to 0. But if someone edits this, the arg order might remain. I will follow your commit text suggestions. Let me think about your comment on 08/13. Thanks! -- Dana L. How danahow@gmail.com +1 650 804 5991 cell ``` ## Junio C Hamano, 2007-04-06 00:29 Subject: Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 Message-ID: <7vvegal506.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vvegal506.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <56b7f5510704051608g4bf9ffb7t9f94b532c7a0ebdf@mail.gmail.com> ``` "Dana How" writes: > Concerning SEEK_SET, > you are correct: my unistd.h #define's SEEK_SET to 0. My understanding of the history is back when there was seek() but not lseek() nor SEEK_X macros, everybody passed bare integers and it was quite well established that 0 meant what SEEK_SET means now, so I would be quite surprised if there is _any_ platform that defines SEEK_SET to anything but zero. Having said that, I wonder if this kind of thing can be caught with sparse. We should be able to express (or perhaps sparse could have a built-in rule that says) something like "although the standard says the third parameter to lseek() can be any integer, you should not use anything but SEEK_X preprocessor macros; also SEEK_X preprocessor macros, although they are typically just "#define"s of small integer literals, can never appear anywhere other than the third parameter as seek() and lseek()". I realize that there needs some data-flow analysis to allow a code like this, though: int move_to(int fd, ofs_t offset, int relative) { int whence = SEEK_SET; if (relative) { whence = SEEK_CUR; offset += relative; } return lseek(fd, offset, whence) != (off_t)-1; } > But if someone edits this, the arg order might remain. Very true, and that is why I said I am going to take the patch. ``` ## Nicolas Pitre, 2007-04-06 00:35 Subject: Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0704052033350.28181%40xanadu.home In-Reply-To: <56b7f5510704051524p28eafc18mae3131ef13cdabfa@mail.gmail.com> ``` This patch looks completely wrong to me. Please explain why you're doing this? Nicolas ``` ## Nicolas Pitre, 2007-04-06 00:45 Subject: Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7 Message-ID: URL: https://gitlist.dev/e/alpine.LFD.0.98.0704052045070.28181%40xanadu.home In-Reply-To: ``` On Thu, 5 Apr 2007, Nicolas Pitre wrote: > > This patch looks completely wrong to me. Please explain why you're > doing this? Never mind. I see that Junio and you already cleared that one. Nicolas ```