Re: [PATCH] git: use U to denote unsigned to prevent UB

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 2, 2025, 21:33 UTC
Message-ID: <xmqq5xmwuchl.fsf@gitster.g>
In-Reply-To: <pull.1849.git.git.1734488549111.gitgitgadget@gmail.com>

"AreaZR via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Seija Kijin <doremylover123@gmail.com>
>
> 1 << can be UB if 1 ends up overflowing and
> being assigned to an unsigned int or long.

 * Spell out what you meant by "UB".

 * "1 ends up overflowing"?  One is one; as long as you have two
   bits, it won't overflow.  This needs rewriting.

Show 10 quoted lines

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5e5afa0f267..a636e71e05c 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -223,7 +223,7 @@ static int check_stages(unsigned stages, const struct cache_entry *ce, int pos)
>  		ce = the_repository->index->cache[pos];
>  		if (strcmp(name, ce->name))
>  			break;
> -		seen |= (1 << ce_stage(ce));
> +		seen |= (1U << ce_stage(ce));

Here "seen" is "unsigned" initialized to 0. Matching the type of the value that is assigned with an explicit U does make sense, but as Jonathan already pointed out elsewhere, as ce_stage() cannot be more than 3, this would never overflow.

Show 10 quoted lines

> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index c5ed472967a..d0104dfa0c7 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -270,13 +270,13 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
>  	unsigned dirmask = 0, mask = 0;
>  
>  	for (i = 0; i < 3; i++) {
> -		mask |= (1 << i);
> +		mask |= (1U << i);

Ditto.

Show 7 quoted lines

>  		/*
>  		 * Treat missing entries as directories so that we return
>  		 * after unresolved_directory has handled this.
>  		 */
>  		if (!n[i].mode || S_ISDIR(n[i].mode))
> -			dirmask |= (1 << i);
> +			dirmask |= (1U << i);

Ditto.

There are a few instances that left shifts by 31-bit, which requires (1 * 2 ** 31) to be representable in the result type of signed int if we want to avoid an undefined behaviour,, but even if your signed int were wider than 32-bit, it is a good hygiene to write your bit shift as (1U << shift_count).

So I am not opposed to these changes. The guiding principle should probably be "bit patterns should by unsigned by default, unless you have a strong and valid reason to use signed" (and the only single plausible reason being when you take advantage of fact that the sign bit is propagated if you shift right); as most of the changed code paths do deal with a signed result that is representable and does not risk any undefined behaviour, it is an inappropriate rationale to justify this particular patch, I would think.