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". 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
