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

From: shejialuo <shejialuo@gmail.com>
Date: Nov 30, 2024, 10:44 UTC
Message-ID: <Z0rsm8X0jkjtrjR8@ArchLinux>
In-Reply-To: <20241129-pks-sign-compare-v1-6-fc406b984bc9@pks.im>

On Fri, Nov 29, 2024 at 02:13:27PM +0100, Patrick Steinhardt wrote:

Show 19 quoted lines

> We have several cases in our codebase where the ternary operator changes
> signedness from a signed integer type to an unsigned one. This causes
> warnings with `-Wsign-compare`. Generally, we seem to have three classes
> of this in our codebase:
> 
>   - Cases where we know that the result is well-formed, e.g. when
>     indexing into strings to determine the length of substrings.
> 
>   - Cases where we want `-1` to mean "unlimited", counting on the
>     wrap-around.
> 
>   - Cases where we may indeed run into problems when one of the
>     statements returns a value that is too big.
> 
> Out of these only the last class is a bit worrying, but we can address
> it by adding a call to `cast_size_t_to_int()`. Like this we're better
> protected in case we have unexpectedly huge input as we'd die instead of
> silently doing the wrong thing.
>

That's good, without using "case_size_t_to_int", we rely on the compiler to do the conversion, which is bad. Now we will die the program.

[snip]

Show 10 quoted lines

> diff --git a/builtin/patch-id.c b/builtin/patch-id.c
> index 56585757477911c96bbb9ef2cf3710691b8e744e..87fa586c4d552ba61cd2ac2cf079d68241eb3275 100644
> --- a/builtin/patch-id.c
> +++ b/builtin/patch-id.c
> @@ -163,7 +163,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
>  			after--;
>  
>  		/* Add line to hash algo (possibly removing whitespace) */
> -		len = verbatim ? strlen(line) : remove_space(line);
> +		len = verbatim ? cast_size_t_to_int(strlen(line)) : remove_space(line);

Here, we convert the unsigned "strelen(line)" because the `len` is a signed value. In the first glance, I think we should change the return type of "remove_space" to "size_t", because it will always return the unsigned value.

However, in our codebase, we use "int" as the return value in the most situations. So, this change make senses.

>  		patchlen += len;
>  		the_hash_algo->update_fn(&ctx, line, len);
>  	}

Show 10 quoted lines

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

Show 16 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?

Show 23 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.

Show 6 quoted lines

>  	while (count--) {
>  		next = next * 1103515245 + 12345;
> 
> -- 
> 2.47.0.366.g5daf58cba8.dirty
>

Thanks, Jialuo