Questionable behavior of strtoi(3bsd) / strtou(3bsd)
Alejandro Colomar
alx at kernel.org
Sun Jan 7 21:32:20 UTC 2024
Hi,
On Sun, Jan 07, 2024 at 09:23:51PM +0100, Alejandro Colomar wrote:
> Sure; will do. I'll send a few more emails to this thread with the
> results of my search in NetBSD's code, and when I have all the info I
> want, I'll file a bug there.
Here are some results of the investigation in the NetBSD tree.
I first found (hopefully) all calls to these functions that check either
ERANGE or ENOTSUP. Calls that don't check either won't be affected if
strtoi(3bsd) changes, as the only proposed change is to report ERANGE
instead of ENOTSUP in certain cases. I found them with
$ find -type f \
| grep '\.c$' \
| xargs grep -l 'strto[iu]' \
| xargs grep --group-separator '++' -n -A20 '\bstrto[iu] *([^0-9]' \
| tac \
| pcre2grep -C1 -M '(?s)(ENOTSUP|ERANGE).*?strto[iu] *\([^0-9]' \
| tac;
Then I opened every file found above, reviewed the code, and added
comments inline:
diff --git a/external/bsd/blocklist/bin/conf.c b/external/bsd/blocklist/bin/conf.c
index b4390ff0253d..9ceddc9f6b50 100644
--- a/external/bsd/blocklist/bin/conf.c
+++ b/external/bsd/blocklist/bin/conf.c
@@ -156,6 +156,13 @@ conf_getsecs(const char *f, size_t l, bool local, struct conf *c, const char *p)
again:
im = strtoi(p, &ep, 0, 0, INT_MAX, &e);
+ /* BUG!
+ * Let p="0xffffFFFFh"
+ *
+ * It will be parsed as if it were "0x0fffFFFFh", while
+ * "0xffffFFFF" would be rejected as out-of-range.
+ * Changing strtoi(3bsd) would fix this bug.
+ */
if (e == ENOTSUP) {
switch (*ep) {
case 'd':
diff --git a/lib/libc/stdlib/strtonum.c b/lib/libc/stdlib/strtonum.c
index 35ae6d2bda62..68623d0c499b 100644
--- a/lib/libc/stdlib/strtonum.c
+++ b/lib/libc/stdlib/strtonum.c
@@ -62,6 +62,11 @@ strtonum(const char *nptr, long long minval, long long maxval,
return rv;
}
+ /* This code is correct, and would break if strtoi(3bsd) changed.
+ * To prepare for it, we'd need to change the conditional to:
+ *
+ * (e == ERANGE && *end != '\0')
+ */
if (e == ERANGE)
*errstr = (rv == maxval ? "too large" : "too small");
else
diff --git a/lib/libutil/pidfile.c b/lib/libutil/pidfile.c
index 3fb7843e02fa..78f801a9a17d 100644
--- a/lib/libutil/pidfile.c
+++ b/lib/libutil/pidfile.c
@@ -158,6 +158,13 @@ pidfile_read(const char *path)
}
buf[n] = '\0';
pid = (pid_t)strtoi(buf, &eptr, 10, 1, INT_MAX, &error);
+ /* BUG!
+ * Let buf="0\n"
+ *
+ * It will be interpreted as a valid 1, while it should be
+ * rejected as ERANGE, as it would reject buf="0".
+ * Changing strtoi(3bsd) would fix this bug.
+ */
if (error && !(error == ENOTSUP && *eptr == '\n')) {
errno = error;
return -1;
diff --git a/tests/lib/libc/stdlib/t_strtoi.c b/tests/lib/libc/stdlib/t_strtoi.c
index 82d9d5b4f90a..d502a500b338 100644
--- a/tests/lib/libc/stdlib/t_strtoi.c
+++ b/tests/lib/libc/stdlib/t_strtoi.c
@@ -184,6 +184,7 @@ ATF_TC_BODY(strtoi_case, tc)
errno = 0;
rv = strtoi(t[i].str, &end, t[i].base, t[i].lo, t[i].hi, &e);
+ /* No problem here */
if (errno != 0)
atf_tc_fail("strtoi(3) changed errno to %d ('%s')",
e, strerror(e));
diff --git a/usr.sbin/inetd/parse.c b/usr.sbin/inetd/parse.c
index d609f21daace..0dec9ce8b230 100644
--- a/usr.sbin/inetd/parse.c
+++ b/usr.sbin/inetd/parse.c
@@ -717,6 +717,14 @@ do { \
if (rstatus != ERANGE) {
/* For compatibility w/ atoi parsing */
sep->se_service_max = 0;
+ /* Inconsistent behavior:
+ * Let cp1="0xFFFFffffFFFFffff9999z"
+ *
+ * If the trailing "z" wasn't there, the value
+ * would have been SERVTAB_COUNT_MAX instead of
+ * 0, since this conditional wouldn't have
+ * executed.
+ */
}
WRN("Improper \"max\" value '%s', "
diff --git a/usr.sbin/inetd/parse_v2.c b/usr.sbin/inetd/parse_v2.c
index c66d94946e89..45153f431c4c 100644
--- a/usr.sbin/inetd/parse_v2.c
+++ b/usr.sbin/inetd/parse_v2.c
@@ -787,6 +787,12 @@ size_to_bytes(char *arg)
count = (int)strtoi(arg, &tail, 10, 0, INT_MAX, &rstatus);
if (rstatus != 0 && rstatus != ENOTSUP) {
+ /* BUG!
+ * Let arg="-7k"
+ *
+ * count will be 0, and no error will be reported.
+ * Changing strtoi(3bsd) would fix this bug.
+ */
ERR("Invalid buffer size '%s': %s", arg, strerror(rstatus));
return -1;
}
diff --git a/usr.sbin/sysinst/util.c b/usr.sbin/sysinst/util.c
index d3c3e51bdd32..eafb6e6a44f3 100644
--- a/usr.sbin/sysinst/util.c
+++ b/usr.sbin/sysinst/util.c
@@ -2263,6 +2263,13 @@ str_arg_subst(const char *src, size_t argc, const char **argv)
/* $ followed by a correct numeric position? */
n = strtou(p+1, &endp, 10, 0, INT_MAX, &e);
+ /* BUG!
+ *
+ * ENOTSUP hides invalid values that are being parsed as
+ * if they were valid (saturated to [0,INT_MAX]). Those
+ * values would otherwise be ignored, as e==ERANGE.
+ * Changing strtou(3bsd) would fix this bug.
+ */
if ((e == 0 || e == ENOTSUP) && n < argc) {
len += strlen(argv[n]);
len -= endp-p;
@@ -2283,6 +2290,10 @@ str_arg_subst(const char *src, size_t argc, const char **argv)
/* $ followed by a correct numeric position? */
n = strtou(p+1, &endp, 10, 0, INT_MAX, &e);
if ((e == 0 || e == ENOTSUP) && n < argc) {
+ /* BUG!
+ *
+ * Exactly the same as above.
+ */
size_t l = p-last;
memcpy(t, last, l);
t += l;
@@ -2317,6 +2328,10 @@ needs_expanding(const char *src, size_t argc)
/* $ followed by a correct numeric position? */
n = strtou(p+1, &endp, 10, 0, INT_MAX, &e);
+ /* BUG!
+ *
+ * Exactly the same as above.
+ */
if ((e == 0 || e == ENOTSUP) && n < argc)
return true;
}
As you can see, there's only one place where changing strtoi(3bsd) would
break, and it has an easy fix. On the other hand, there are several
bugs, due to precisely this strtoi(3bsd) bug, which would become fixed
by fixing strtoi(3bsd).
I'll report to NetBSD, and hopefully we'll be able to fix this. :)
Cheers,
Alex
--
<https://www.alejandro-colomar.es/>
Looking for a remote C programming job at the moment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/libbsd/attachments/20240107/bb87a152/attachment.sig>
More information about the libbsd
mailing list