Re: [PATCH 1/3] set errno=0 before strtoX calls

[Kyle Lippincott <spectral@google.com>]

On Thu, Aug 1, 2024 at 10:12 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Fri, Aug 02, 2024 at 04:10:51AM +0000, Kyle Lippincott via GitGitGadget wrote:
> > From: Kyle Lippincott <spectral@google.com>
> >
> > To detect conversion failure after calls to functions like `strtod`, one
> > can check `errno == ERANGE`. These functions are not guaranteed to set
> > `errno` to `0` on successful conversion, however. Manual manipulation of
> > `errno` can likely be avoided by checking that the output pointer
> > differs from the input pointer, but that's not how other locations, such
> > as parse.c:139, handle this issue; they set errno to 0 prior to
> > executing the function.
> >
> > For every place I could find a strtoX function with an ERANGE check
> > following it, set `errno = 0;` prior to executing the conversion
> > function.
>
> Makes sense. I've also gone through callsites and couldn't spot any
> additional ones that are broken.
>
> Generally speaking, the interfaces provided by the `strtod()` family of
> functions is just plain awful, and ideally we wouldn't be using them in
> the Git codebase at all without a wrapper. We already do have wrappers
> for a subset of those functions, e.g. `strtol_i()`, which use an out
> pointer to store the result and indicate success via the return value
> instead of via `errno`.
>
> It would be great if we could extend those wrappers to cover all of the
> integer types, convert our code base to use them, and then extend our
> "banned.h" banner. I'm of course not asking you to do that in this patch
> series.
>
> Out of curiosity, why do you hit those errors in your test setup? Do you
> use a special libc that behaves differently than the most common ones?

The second patch in this series fixes the original reason I noticed
the issues in three of the files: our remote test execution service
uses paths that are >128 bytes long, so the getcwd call in
strbuf_getcwd was returning ERANGE once, and then it remained set
since getcwd didn't clear it on success. ref-filter.c was found via
searching, I think that was during the search for `ERANGE`.

>
> Patrick