Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Nov 18, 2025, 14:30 UTC
- Message-ID
- <ca6d99cc-d05c-49fb-ab3c-d7668077d32b@gmail.com>
- In-Reply-To
- <20251118091218.GD529192@coredump.intra.peff.net>
Hi Peff
On 18/11/2025 09:12, Jeff King wrote:
Show 16 quoted lines
> Let's fix it by just parsing the values ourselves with a helper function > that is careful not to go past the end of the buffer. There are a few > behavior changes here that should not matter: > > - We do not consider overflow, as strtol() would. But nor did the > original code. However, we don't trust the value we get from the > on-disk file, and if it says to read 2^30 entries, we would notice > that we do not have that many and bail before reading off the end of > the buffer. > > - Our helper does not skip past extra leading whitespace as strtol() > would, but according to gitformat-index(5) there should not be any. > > - The original quit parsing at a newline or a NUL byte, but now we > insist on a newline (which is what the documentation says, and what > Git has always produced).
I think that sounds reasonable, I've left a couple of comments below.
Show 5 quoted lines
> +static int parse_int(const char **ptr, unsigned long *len_p, int *out)
> +{
> + const char *s = *ptr;
> + unsigned long len = *len_p;
> + int ret = 0;This is signed which means that any overflow is undefined. While the existing code does not check for overflow I think it is well defined in the presence of overflow. It also means parsing INT_MIN is undefined as we parse the value as unsigned and then multiply by -1 if we saw a leading '-'. We shouldn't see any negative values apart from "-1" but given we're changing this code to be more robust in handling malformed input it would be nice if parsing INT_MIN was well defined.
Show 7 quoted lines
> + int sign = 1;
> +
> + while (len && *s == '-') {
> + sign *= -1;
> + s++;
> + len--;
> + }This accepts any number of '-' signs but I believe strtol() only accepts a single sign (the standard says "optionally preceded by a plus or minus sign") so this is a change in behavior from the existing code. I'm not sure we really need to be that accommodating here.
Show 11 quoted lines
> + while (len) {
> + if (!isdigit(*s))
> + break;
> + ret *= 10;
> + ret += *s - '0';
> + s++;
> + len--;
> + }
> +
> + if (s == *ptr)
> + return -1;This accepts "-" as a valid input, as we're tightening up our parsing it would be nice to require a digit after any '-' sign.
> [...]> + buf++; size--;
> + if (parse_int(&buf, &size, &subtree_nr) < 0) > + goto free_return;
This isn't a new problem but if subtree_nr is negative we end up trying to allocate a huge chunk of memory. If that somehow succeeds we then end up calling die("cache-tree: internal error"). The existing code looks safe but it would be nice to die() a bit earlier if subtree_nr is negative.
Thanks
Phillip > + if (!size || *buf != '\n')
> goto free_return;
> buf++; size--;
> if (0 <= it->entry_count) {