Re: [PATCH 06/10] global: fix unsigned integer promotions in ternary statements

From: Patrick Steinhardt <ps@pks.im>
Date: Dec 2, 2024, 07:54 UTC
Message-ID: <Z01nmHnByrx_H1ZA@pks.im>
In-Reply-To: <Z0rsm8X0jkjtrjR8@ArchLinux>

On Sat, Nov 30, 2024 at 06:44:43PM +0800, shejialuo wrote:

Show 16 quoted lines

> On Fri, Nov 29, 2024 at 02:13:27PM +0100, Patrick Steinhardt wrote:
> > diff --git a/gpg-interface.c b/gpg-interface.c
> > index a67d80475bf9d8452de0c3ae9bb08ceeb4c11c4b..e1720361f17e8b3b3315f0a5d93a827e11b2b036 100644
> > --- a/gpg-interface.c
> > +++ b/gpg-interface.c
> > @@ -700,7 +700,7 @@ size_t parse_signed_buffer(const char *buf, size_t size)
> >  			match = len;
> >  
> >  		eol = memchr(buf + len, '\n', size - len);
> > -		len += eol ? eol - (buf + len) + 1 : size - len;
> > +		len += eol ? (size_t) (eol - (buf + len) + 1) : size - len;
> 
> When reading the code, I wonder what if "eol - (buf + len) + 1" is less
> than zero. If so, we would cause underflow. We have created a helper
> function "cast_size_t_to_int". Do we need to create a function to safely
> convert the potential signed integer to "size_t"?

In this case it can't be. I'll explode this commit into several commits and explain things a bit more in-depth.

Show 20 quoted lines

> > diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c
> > index ea9b9b656307d32bdc1f2e15a91793b1dda9c463..31dbe7db4ac61639541f15d262cea64368fec78f 100644
> > --- a/t/helper/test-csprng.c
> > +++ b/t/helper/test-csprng.c
> > @@ -1,5 +1,3 @@
> > -#define DISABLE_SIGN_COMPARE_WARNINGS
> > -
> >  #include "test-tool.h"
> >  #include "git-compat-util.h"
> >  
> > @@ -14,7 +12,7 @@ int cmd__csprng(int argc, const char **argv)
> >  		return 2;
> >  	}
> >  
> > -	count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L;
> > +	count = (argc == 2) ? strtoul(argv[1], NULL, 0) : (unsigned long) -1L;
> 
> Here we use "-1" to represent the unlimited, and the type of `count` is
> unsigned long. So we need to explicitly case the type of "-1L". But this
> is strange, why not just use the "ULONG_MAX" directly?

Yup, good suggestion.

Show 25 quoted lines

> >  
> >  	while (count) {
> >  		unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);
> > diff --git a/t/helper/test-genrandom.c b/t/helper/test-genrandom.c
> > index 5b51e6648d8e698b09f400efcf67a0708c226e9d..efca20e7efff46bf66c2b8888ce88db02e545cd5 100644
> > --- a/t/helper/test-genrandom.c
> > +++ b/t/helper/test-genrandom.c
> > @@ -4,8 +4,6 @@
> >   * Copyright (C) 2007 by Nicolas Pitre, licensed under the GPL version 2.
> >   */
> >  
> > -#define DISABLE_SIGN_COMPARE_WARNINGS
> > -
> >  #include "test-tool.h"
> >  #include "git-compat-util.h"
> >  
> > @@ -24,7 +22,7 @@ int cmd__genrandom(int argc, const char **argv)
> >  		next = next * 11 + *c;
> >  	} while (*c++);
> >  
> > -	count = (argc == 3) ? strtoul(argv[2], NULL, 0) : -1L;
> > +	count = (argc == 3) ? strtoul(argv[2], NULL, 0) : (unsigned long) -1L;
> >  
> 
> Similar with the above comment.

And this one, too.

Thanks!

Patrick