Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

From: Jeff King <peff@peff.net>
Date: Dec 27, 2024, 21:37 UTC
Message-ID: <20241227213729.GA796141@coredump.intra.peff.net>
In-Reply-To: <xmqq7c7kubx8.fsf@gitster.g>

On Fri, Dec 27, 2024 at 12:08:03PM -0800, Junio C Hamano wrote:

Show 38 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Error: shallow.c:537:32: comparison of integer expressions of different signedness:
> > 'unsigned int' and 'int' [-Werror=sign-compare]
> >
> >     537 |  if (!info->pool_count || size > info->end - info->free) {
> >
> > I didn't dig deeper than this.
> 
> What we want to express seems to be:
> 
>     If the region between "end" and "free" is smaller than the
>     required "size" then we are in trouble.
> 
> which can be said more naturally with
> 
>     If the region of size "size" starting at "free" pointer overruns the
>     "end" pointer, we are in trouble.
> 
> So perhaps something like this would help?  We are no longer making
> a comparison between two integers with this rewrite.
> 
>  shallow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git c/shallow.c w/shallow.c
> index b8fcfbef0f..b54244ffa9 100644
> --- c/shallow.c
> +++ w/shallow.c
> @@ -534,7 +534,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
>  	unsigned nr = DIV_ROUND_UP(info->nr_bits, 32);
>  	unsigned size = nr * sizeof(uint32_t);
>  	void *p;
> -	if (!info->pool_count || size > info->end - info->free) {
> +	if (!info->pool_count || info->end < info->free + size) {
>  		if (size > POOL_SIZE)
>  			BUG("pool size too small for %d in paint_alloc()",
>  			    size);

I doubt it is a practical problem in this instance, but as a general rule, I think bounds checks like this should avoid using addition because it can lead to overflow. In this code, imagine that the free pool was allocated near the end of the address space, but somebody asked for a very large "size", causing the addition to wrap around.

I think even without that overflow this might be undefined behavior, because "info->free + size" is forming a pointer that may be outside the allocated array.

So as a general rule, bounds comparisons like this should be formulated as "how much free space do we have" compared to "how much are we asking for". And there "info->end - info->free" is the right way to ask the first half of that question.

Now where it gets weird with -Wsign-compare is that the pointer difference is giving us a signed value. Which makes sense in the general case (you could ask for "info->free - info->end", for example). But we know that it will always be non-negative because we know that info->free is <= info->end, coming from earlier in the same allocation.

I doubt there is a way to tell the compiler that (or that a compiler could even switch to an unsigned ptrdiff type if it knew that). But I wonder if there is a generalized helper we can devise that would avoid simply casting here. I guess that could be a checked cast like:

  static inline size_t ptrdiff_to_size(ptrdiff_t v)
  {
	if (v < 0)
		BUG("surprising negative value: %"PRIdMAX, v);
	return (size_t)v;
  }

or even:

  static inline bool has_space(const void *vs, const void *ve, size_t want)
  {
	const char *s = vs, e = ve;
	return want <= ptrdiff_to_size(ve - vs);
  }

I don't love hiding basic things like this behind macros or inlines. But allocation and bounds comparisons do have gotchas (especially against an adversary that can try to create pathological situations). Maybe it's worth having an easy way to do them safely without having to think about each one. I dunno.

-Peff