Re: [PATCH] hex: use unsigned index for ring buffer

From: Jeff King <peff@peff.net>
Date: Oct 23, 2016, 09:11 UTC
Message-ID: <20161023091146.p2kmqvgwxdf77dnn@sigill.intra.peff.net>
In-Reply-To: <ebf769d4-771f-499e-c7fc-f0377d8df18e@web.de>

On Sun, Oct 23, 2016 at 11:00:48AM +0200, René Scharfe wrote:

Show 8 quoted lines

> Overflow is defined for unsigned integers, but not for signed ones.
> Make the ring buffer index in sha1_to_hex() unsigned to be on the
> safe side.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> Hard to trigger, but probably even harder to diagnose once someone
> somehow manages to do it on some uncommon architecture.

Indeed. If we are worried about overflow, we would also want to assume that it wraps at a multiple of 4, but that is probably a sane assumption.

Show 13 quoted lines

> diff --git a/hex.c b/hex.c
> index ab2610e..8c6c189 100644
> --- a/hex.c
> +++ b/hex.c
> @@ -76,7 +76,7 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)
>  
>  char *sha1_to_hex(const unsigned char *sha1)
>  {
> -	static int bufno;
> +	static unsigned int bufno;
>  	static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
>  	return sha1_to_hex_r(hexbuffer[3 & ++bufno], sha1);
>  }

I wonder if just truncating bufno would be conceptually simpler (albeit longer):

  bufno++;
  bufno &= 3;
  return sha1_to_hex_r(hexbuffer[bufno], sha1);

You could also write the second line like:

  bufno %= ARRAY_SIZE(hexbuffer);

which is less magical (right now the set of buffers must be a power of 2). I expect the compiler could turn that into a bitmask itself.

I'm fine with any of the options. I guess you'd want a similar patch for find_unique_abbrev on top of jk/no-looking-at-dotgit-outside-repo.

-Peff