Re: [PATCH v2 02/13] msvc: avoid using minus operator on unsigned types

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 3, 2019, 22:44 UTC
Message-ID: <xmqqy2y1a1qk.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <8800320590e4d7218a80f80abca23a7f44b8747d.1569837329.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines

> While at it, we take care of reporting overflows (which are unlikely,
> but hey, defensive programming is good!).
>
> We _also_ take pains of casting the unsigned value to signed: otherwise,
> the signed operand (i.e. the `-1`) would be cast to unsigned before
> doing the arithmetic.

These three look good and too similar to each other, which makes me wonder if we want to allow them simply write

	return insert_pos_as_negative_offset(nr);

with something like

	static int insert_pos_as_negative_offset(uintmax_t nr)
	{
		if (INT_MAX < nr)
			die("overflow: -1 - %"PRIuMAX, nr);
		return -1 - (int)nr;
	}

to avoid repetition.

Show 64 quoted lines

> Helped-by: Denton Liu <liu.denton@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  read-cache.c  |  9 ++++++---
>  sha1-lookup.c | 12 +++++++++---
>  2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index c701f7f8b8..97745c2a31 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1275,8 +1275,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
>  	 * we can avoid searching for it.
>  	 */
>  	if (istate->cache_nr > 0 &&
> -		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
> -		pos = -istate->cache_nr - 1;
> +		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0) {
> +		if (istate->cache_nr > INT_MAX)
> +			die("overflow: -1 - %u", istate->cache_nr);
> +		pos = -1 - (int)istate->cache_nr;
> +	}
>  	else
>  		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
>  
> @@ -1894,7 +1897,7 @@ static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
>  	/*
>  	 * Account for potential alignment differences.
>  	 */
> -	per_entry += align_padding_size(sizeof(struct cache_entry), -sizeof(struct ondisk_cache_entry));
> +	per_entry += align_padding_size(per_entry, 0);
>  	return ondisk_size + entries * per_entry;
>  }
>  
> diff --git a/sha1-lookup.c b/sha1-lookup.c
> index 796ab68da8..bb786b5420 100644
> --- a/sha1-lookup.c
> +++ b/sha1-lookup.c
> @@ -69,8 +69,12 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
>  			miv = take2(sha1 + ofs);
>  			if (miv < lov)
>  				return -1;
> -			if (hiv < miv)
> -				return -1 - nr;
> +			if (hiv < miv) {
> +				if (nr > INT_MAX)
> +					die("overflow: -1 - %"PRIuMAX,
> +					    (uintmax_t)nr);
> +				return -1 - (int)nr;
> +			}
>  			if (lov != hiv) {
>  				/*
>  				 * At this point miv could be equal
> @@ -97,7 +101,9 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
>  			lo = mi + 1;
>  		mi = lo + (hi - lo) / 2;
>  	} while (lo < hi);
> -	return -lo-1;
> +	if (nr > INT_MAX)
> +		die("overflow: -1 - %"PRIuMAX, (uintmax_t)lo);
> +	return -1 - (int)lo;
>  }
>  
>  int bsearch_hash(const unsigned char *sha1, const uint32_t *fanout_nbo,