On Fri, Dec 27, 2024 at 12:08:03PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> 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
