Re: [PATCH 1/2] strbuf: use st_add3() in strbuf_grow()

["René Scharfe" <l.s.r@web.de>]

On 5/15/26 6:36 AM, Jeff King wrote:
> On Thu, May 14, 2026 at 10:13:19PM +0200, René Scharfe wrote:
> 
>> Hmm, alloc_nr() doesn't do any overflow checking.  It should, though,
>> shouldn't it?
> 
> Yes, probably. It's a known blind spot in the overflow checking, but
> I think is OK in practice because:
> 
>   1. We are growing an existing buffer by ~3/2. So even with ordering
>      the multiplication first, an overflow implies that you have a
>      single buffer consuming ~1/3 of your address space.
> 
>      On 64-bit systems that's impractically large, and on 32-bit systems I
>      think you generally run into fragmentation and address-space issues
>      first.
> 
>   2. If alloc_nr(alloc) is less than the desired nr, we just use that nr
>      directly. So even if we did overflow, I think the result is
>      too-slow allocation, and not a buffer overflow.
> > But it would be nice to be less hand-wavy. One of the reasons I hadn't
> dug into it further is that I wanted to start making use of intrinsics
> to avoid slowdowns. But since you're already doing that (and finding
> that the compiler was doing the fast thing anyway!) it might be a good
> time to make the jump.

Didn't look at __builtin_mul_overflow() in detail; its situation could
be  different than for __builtin_add_overflow(), which turned out to be
unnecessary on x64.

> That's all assuming that no overflow happens before ALLOC_GROW() gets
> the values. We also tend to do unchecked computions for the "nr" field
> there, but it's usually just "nr_foo + 1", so the same logic applies:
> you'd have to have an existing array consuming the entire address space
> minus one byte to trigger an overflow.
The use in read-cache.c::do_read_index() looks odd.  Has been present
since commit one.  Is the point that it over-allocates to have room for
additions right from the start?  For read-only commands this only wastes
memory, no?

René