[PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7

[PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7

[Dana How]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

---
 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

[-- Attachment #2: 0002-declare-overflow-during-base128-decoding-when-1-MSB.patch.txt --]
[-- Type: text/plain, Size: 2446 bytes --]

From 90937b722f391abe1cbc035c29b33f500c389334 Mon Sep 17 00:00:00 2001
From: Dana How <how@deathvalley.cswitch.com>
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

Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7

[Junio C Hamano]

"Dana How" <danahow@gmail.com> 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?

Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7

[Dana How]

On 4/5/07, Junio C Hamano <junkio@cox.net> wrote:
> "Dana How" <danahow@gmail.com> 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

Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7

[Junio C Hamano]

"Dana How" <danahow@gmail.com> 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.

Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7

[Nicolas Pitre]

This patch looks completely wrong to me.  Please explain why you're 
doing this?


Nicolas

Re: [PATCH 02/13] declare overflow during base128 decoding when 1 MSB nonzero, not 7

[Nicolas Pitre]

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