threads / patch / 5359

patchFix a comparison bug in diff-delta.c

Subject: [PATCH] Fix a comparison bug in diff-delta.c

## tl;dr

3 messages between Aug 23, 2006 and Aug 23, 2006. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Pierre Habouzit· Aug 23, 2006, 02:32 UTC · lore

(1 << i) < hspace is compared in the `int` space rather that in the unsigned one. the result will be wrong if hspace is between 0x40000000 and 0x80000000.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

  I'm currently trying to make git compile with more strict gcc flags
(-g -O2 -Wall -Wextra -Wno-unused -Werror to be precise) and I've
spotted a first bug due to a signed/unsigned comparison.

  If I do understand that bit of code, it should not bite a lot of
people, but this is still a bug ;)

 diff-delta.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to diff-delta.c +1 −1

diff --git a/diff-delta.c b/diff-delta.c
index 7da9205..a1fadc9 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -152,7 +152,7 @@ struct delta_index * create_delta_index(
 	   initialization in create_delta(). */
 	entries = (bufsize - 1)  / RABIN_WINDOW;
 	hsize = entries / 4;
-	for (i = 4; (1 << i) < hsize && i < 31; i++);
+	for (i = 4; (unsigned)(1 << i) < hsize && i < 31; i++);
 	hsize = 1 << i;
 	hmask = hsize - 1;

-- 
1.4.1.1

Pierre Habouzit· Aug 23, 2006, 02:38 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Fix a comparison bug in diff-delta.c

Le mer 23 août 2006 04:32, Pierre Habouzit a écrit :

>   I'm currently trying to make git compile with more strict gcc flags
> (-g -O2 -Wall -Wextra -Wno-unused -Werror to be precise) and I've
> spotted a first bug due to a signed/unsigned comparison.

still in the same kind of "problems" sha1_name.c:512 has:

		if (active_nr < 0)
			return -1;

but active_nr is defined as an unsigned (in cache.h/cache-read.c).

Also note that there is a *lot* of signed/unsigned comparisons due to the fact that it's unsigned, and that it's often compared to positions that are ints (I assume pos need the negative values to mean "error").

so either those two lines are too much, or active_nr has to be a plain signed int. I'm not used to git sources enough yet to be able to make the right decision.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Nicolas Pitre· Aug 23, 2006, 03:17 UTC · re: Pierre Habouzit · lore

Re: [PATCH] Fix a comparison bug in diff-delta.c

On Wed, 23 Aug 2006, Pierre Habouzit wrote:

> (1 << i) < hspace is compared in the `int` space rather that in the unsigned one.
> the result will be wrong if hspace is between  0x40000000 and 0x80000000.
> 
> Signed-off-by: Pierre Habouzit <madcoder@debian.org>

Could you use (1u << i) instead of (unsigned)(1 << i) ? That looks prettier to me at least.

Nicolas