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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 2, 2024, 05:12 UTC
Message-ID: <ZqxqtIJi4-xBL9Sj@tanuki>
In-Reply-To: <4dbd0bec40a0f9fd715e07a56bc6f12c4b29a83c.1722571853.git.gitgitgadget@gmail.com>

On Fri, Aug 02, 2024 at 04:10:51AM +0000, Kyle Lippincott via GitGitGadget wrote:

Show 13 quoted lines

> 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?

Patrick