* [PATCH] Fix a comparison bug in diff-delta.c @ 2006-08-23 2:32 Pierre Habouzit 2006-08-23 2:38 ` Pierre Habouzit 2006-08-23 3:17 ` Nicolas Pitre 0 siblings, 2 replies; 6+ messages in thread From: Pierre Habouzit @ 2006-08-23 2:32 UTC (permalink / raw) To: git; +Cc: Pierre Habouzit (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(-) 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix a comparison bug in diff-delta.c 2006-08-23 2:32 [PATCH] Fix a comparison bug in diff-delta.c Pierre Habouzit @ 2006-08-23 2:38 ` Pierre Habouzit 2006-08-23 3:17 ` Nicolas Pitre 1 sibling, 0 replies; 6+ messages in thread From: Pierre Habouzit @ 2006-08-23 2:38 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 993 bytes --] 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 [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix a comparison bug in diff-delta.c 2006-08-23 2:32 [PATCH] Fix a comparison bug in diff-delta.c Pierre Habouzit 2006-08-23 2:38 ` Pierre Habouzit @ 2006-08-23 3:17 ` Nicolas Pitre 1 sibling, 0 replies; 6+ messages in thread From: Nicolas Pitre @ 2006-08-23 3:17 UTC (permalink / raw) To: Pierre Habouzit; +Cc: git 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Fix a comparison bug in diff-delta.c @ 2006-08-23 9:17 Pierre Habouzit 2006-08-23 13:45 ` Johannes Schindelin 0 siblings, 1 reply; 6+ messages in thread From: Pierre Habouzit @ 2006-08-23 9:17 UTC (permalink / raw) To: git; +Cc: Pierre Habouzit (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> --- This is a replacement for <1156300368160-git-send-email-madcoder@debian.org> avoiding the ugly cast, and using 1u instead. diff-delta.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/diff-delta.c b/diff-delta.c index 7da9205..51e2e56 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; (1u << i) < hsize && i < 31; i++); hsize = 1 << i; hmask = hsize - 1; -- 1.4.1.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix a comparison bug in diff-delta.c 2006-08-23 9:17 Pierre Habouzit @ 2006-08-23 13:45 ` Johannes Schindelin 2006-08-23 14:31 ` Pierre Habouzit 0 siblings, 1 reply; 6+ messages in thread From: Johannes Schindelin @ 2006-08-23 13:45 UTC (permalink / raw) To: Pierre Habouzit; +Cc: git Hi, On Wed, 23 Aug 2006, Pierre Habouzit wrote: > - for (i = 4; (1 << i) < hsize && i < 31; i++); > + for (i = 4; (1u << i) < hsize && i < 31; i++); The variable i never takes on the value 31 (or any higher value), so there is no bug here. Unless you port git to a system where an int has less than 32 bit. Ciao, Dscho ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix a comparison bug in diff-delta.c 2006-08-23 13:45 ` Johannes Schindelin @ 2006-08-23 14:31 ` Pierre Habouzit 0 siblings, 0 replies; 6+ messages in thread From: Pierre Habouzit @ 2006-08-23 14:31 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git [-- Attachment #1: Type: text/plain, Size: 696 bytes --] Le mer 23 août 2006 15:45, Johannes Schindelin a écrit : > Hi, > > On Wed, 23 Aug 2006, Pierre Habouzit wrote: > > - for (i = 4; (1 << i) < hsize && i < 31; i++); > > + for (i = 4; (1u << i) < hsize && i < 31; i++); > > The variable i never takes on the value 31 (or any higher value), so > there is no bug here. Unless you port git to a system where an int > has less than 32 bit. that remains quite tasteless though, and maybe that patch can be integrated to the "cleanup" series I sent later today. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2006-08-23 14:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2006-08-23 2:32 [PATCH] Fix a comparison bug in diff-delta.c Pierre Habouzit 2006-08-23 2:38 ` Pierre Habouzit 2006-08-23 3:17 ` Nicolas Pitre -- strict thread matches above, loose matches on Subject: below -- 2006-08-23 9:17 Pierre Habouzit 2006-08-23 13:45 ` Johannes Schindelin 2006-08-23 14:31 ` Pierre Habouzit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).