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

Commit Message

Kyle Lippincott Aug. 2, 2024, 4:10 a.m. UTC

From: Kyle Lippincott <[e-mail]>

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.

Signed-off-by: Kyle Lippincott <[e-mail]>
---
 builtin/get-tar-commit-id.c | 1 +
 ref-filter.c                | 1 +
 t/helper/test-json-writer.c | 2 ++
 t/helper/test-trace2.c      | 1 +
 4 files changed, 5 insertions(+)

Comments

Patrick Steinhardt Aug. 2, 2024, 5:12 a.m. UTC | #1

On Fri, Aug 02, 2024 at 04:10:51AM +0000, Kyle Lippincott via GitGitGadget wrote:
> From: Kyle Lippincott <[e-mail]>
> 
> 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

Kyle Lippincott Aug. 2, 2024, 6:15 a.m. UTC | #2

On Thu, Aug 1, 2024 at 10:12 PM Patrick Steinhardt <[e-mail]> wrote:
>
> On Fri, Aug 02, 2024 at 04:10:51AM +0000, Kyle Lippincott via GitGitGadget wrote:
> > From: Kyle Lippincott <[e-mail]>
> >
> > 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

Junio C Hamano Aug. 2, 2024, 3:01 p.m. UTC | #3

Patrick Steinhardt <[e-mail]> writes:

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

A good #leftoverbits material.

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

;-)

Patch

diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index 66a7389f9f4..7195a072edc 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -35,6 +35,7 @@  int cmd_get_tar_commit_id(int argc, const char **argv UNUSED, const char *prefix
 	if (header->typeflag[0] != TYPEFLAG_GLOBAL_HEADER)
 		return 1;
 
+	errno = 0;
 	len = strtol(content, &end, 10);
 	if (errno == ERANGE || end == content || len < 0)
 		return 1;
diff --git a/ref-filter.c b/ref-filter.c
index 8c5e673fc0a..54880a2497a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1628,6 +1628,7 @@  static void grab_date(const char *buf, struct atom_value *v, const char *atomnam
 	timestamp = parse_timestamp(eoemail + 2, &zone, 10);
 	if (timestamp == TIME_MAX)
 		goto bad;
+	errno = 0;
 	tz = strtol(zone, NULL, 10);
 	if ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)
 		goto bad;
diff --git a/t/helper/test-json-writer.c b/t/helper/test-json-writer.c
index ed52eb76bfc..a288069b04c 100644
--- a/t/helper/test-json-writer.c
+++ b/t/helper/test-json-writer.c
@@ -415,6 +415,7 @@  static void get_i(struct line *line, intmax_t *s_in)
 
 	get_s(line, &s);
 
+	errno = 0;
 	*s_in = strtol(s, &endptr, 10);
 	if (*endptr || errno == ERANGE)
 		die("line[%d]: invalid integer value", line->nr);
@@ -427,6 +428,7 @@  static void get_d(struct line *line, double *s_in)
 
 	get_s(line, &s);
 
+	errno = 0;
 	*s_in = strtod(s, &endptr);
 	if (*endptr || errno == ERANGE)
 		die("line[%d]: invalid float value", line->nr);
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index cd955ec63e9..c588c273ce7 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -26,6 +26,7 @@  static int get_i(int *p_value, const char *data)
 	if (!data || !*data)
 		return MyError;
 
+	errno = 0;
 	*p_value = strtol(data, &endptr, 10);
 	if (*endptr || errno == ERANGE)
 		return MyError;