Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`
- From
Jeff King <peff@peff.net>
- Date
- May 16, 2025, 15:42 UTC
- Message-ID
- <20250516154250.GA26009@coredump.intra.peff.net>
- In-Reply-To
- <20250516134054.217202-1-phillip.wood123@gmail.com>
On Fri, May 16, 2025 at 02:40:54PM +0100, Phillip Wood wrote:
Show 12 quoted lines
> On 16/05/2025 11:11, Phillip Wood wrote: > > > I had a similar thought, though to make sure that we parsed 64 bit > > values correctly on windows so we'd need something based on strtoumax() > > I think. > > Perhaps something like the diff below which adds strtoul_u64() in a > similar vein to strtoul_ui(). I think it's debatable whether we really > want to skip leading whitespace so we could perhaps tighten things up > by replacing "if (strchr(s, '-'))" with "if (!isdigit(*s))" though > that would mean this function would behave slightly differently to > strtoul_ui().
It feels like we would had to have dealt with this before for other large values. But poking around at a few obvious suspects (e.g., packSizeLimit), it looks like they are all constrained to "unsigned long".
So yeah, we probably do need something new. IMHO we should probably have repo_config_get_u64() or similar (with the appropriate underlying helpers as well) as use it here. But I am happy with any solution.
And I do agree that we should consider banning *scanf(). With numeric placeholders I don't think they're a security problem (though they are easy to get wrnog, as this discussion shows). But using them with "%s" should generally be disallowed.
There is an fscanf() in builtin/gc.c that uses "%s", but it is careful to construct a custom format string that limits the string size. Yuck. The usual thing in our code base would be to read into a buffer and parse from there.
-Peff