Re: [PATCH 1/3] set errno=0 before strtoX calls
- From
Kyle Lippincott <spectral@google.com>
- Date
- Aug 2, 2024, 06:15 UTC
- Message-ID
- <CAO_smViSG27KrtE7hgq1GAzUYSoKFgrQymRYg-aKJqm4UW9DUg@mail.gmail.com>
- In-Reply-To
- <ZqxqtIJi4-xBL9Sj@tanuki>
On Thu, Aug 1, 2024 at 10:12 PM Patrick Steinhardt <ps@pks.im> wrote:
Show 33 quoted lines
> > 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