On 16/05/2025 16:42, Jeff King wrote:
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.
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
