On Thu, Aug 1, 2024 at 10:12 PM Patrick Steinhardt <ps@xxxxxx> wrote: > > On Fri, Aug 02, 2024 at 04:10:51AM +0000, Kyle Lippincott via GitGitGadget wrote: > > From: Kyle Lippincott <spectral@xxxxxxxxxx> > > > > 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
