From: Taylor Blau <me@ttaylorr.com>
Date: Thu, 13 Nov 2025 03:09:22 GMT
Subject: Re: [PATCH 4/9] cache-tree: avoid strtol() on non-string buffer
Message-ID: <aRVL4iptEeLm/+cs@nand.local>
In-Reply-To: <aRRuzrmbJBW8q4Dd@pks.im>

On Wed, Nov 12, 2025 at 12:26:06PM +0100, Patrick Steinhardt wrote:
> Hm. I'm not a huge fan of not having any error handling at all. It just
> feels way too fragile for my taste:
>
>   - As you mention we don't detect overflows, as we would detect them at
>     a later point in time when trying to access index entries at invalid
>     offsets. But if the input is crafted in a way that the overflow ends
>     up with a reasonable index entry we might just as well _not_ detect
>     that an overflow has happened and end up using the wrong index
>     entry.
>
>   - We don't verify that we even have a number in the first place. We'd
>     simply return "0" in that case and not advance the pointer. This is
>     fine though as we verify that the returned size is non-zero, so we'd
>     detect this case.
>
> I'd much rather prefer to have an interface similar to `git_parse_int()`
> and related functions, which are way easier to use compared to the likes
> of `stroi()`.

Those git_parse_XYZ() functions all end up calling either
git_parse_signed() or git_parse_unsigned() under the hood, which bolts
on our k/m/g suffixes, which we probably don't want here when parsing an
on-disk format.

I don't have a strong opinion here, though I tend to agree with
Patrick's thinking above (with the exception of the suffix thing that I
pointed to earlier).

Thanks,
Taylor