Re: [PATCH 10/11] bundle-uri: avoid using undefined output of `sscanf()`
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- May 19, 2025, 09:03 UTC
- Message-ID
- <32de776c-9f15-4030-9f78-76384dd6dbb1@gmail.com>
- In-Reply-To
- <20250516154250.GA26009@coredump.intra.peff.net>
On 16/05/2025 16:42, Jeff King wrote:
Show 19 quoted lines
> On Fri, May 16, 2025 at 02:40:54PM +0100, Phillip Wood wrote: > >> 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".
I was surprised by that as well
> 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.
I think repo_config_get_ulong() and friends all accept a multiplier suffix. That makes sense for things like packSizeLimit but here we're expecting a bare integer. It probably doesn't really matter but as one of the code paths parses a file that comes from the bundle server we might want to be as strict as we can be.
Show 7 quoted lines
> 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.
Yes that looks pretty horrid
Phillip
> The usual thing in our code base would be to read into a buffer and > parse from there. > > -Peff