Reject negative numbers in strtoul(3) #875

alejandro-colomar · 2023-12-17T21:13:48Z

No description provided.

alejandro-colomar · 2023-12-29T21:36:20Z

!! This may break some users (already broken users, that is).

We'll need at least a release note.

alejandro-colomar · 2023-12-30T12:59:40Z

Queued after #863 (done)

a range (useful for lastlog).

lib/atoi/strtou_noneg.h

alejandro-colomar · 2024-01-16T00:38:30Z

v2 changes:

Rebase to master.

$ git range-diff master..gh/strtou shadow/master..strtou
1:  6ac96ba4 = 1:  fe41ee9a lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
2:  8423d2e2 ! 2:  4448e24d lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
    @@ lib/getulong.c
      #include "prototypes.h"
      
      
    -@@ lib/getulong.c: getulong(const char *numstr, /*@out@*/unsigned long *result)
    +@@ lib/getulong.c: getulong(const char *restrict numstr, unsigned long *restrict result)
        unsigned long  val;
      
        errno = 0;
3:  1c097bac = 3:  6735732d tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

alejandro-colomar · 2024-01-16T00:42:26Z

v3 changes:

Use ATTR_ACCESS()

$ git range-diff shadow/master gh/strtou strtou
1:  fe41ee9a ! 1:  e3afc5e9 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
    @@ lib/atoi/strtou_noneg.h (new)
     +#include "attr.h"
     +
     +
    -+ATTR_STRING(1)
    ++ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
     +inline unsigned long strtoul_noneg(const char *nptr,
     +    char **restrict endptr, int base);
    -+ATTR_STRING(1)
    ++ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
     +inline unsigned long long strtoull_noneg(const char *nptr,
     +    char **restrict endptr, int base);
     +
2:  4448e24d = 2:  15f03a7c lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
3:  6735732d = 3:  bdd8f033 tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

lib/atoi/strtou_noneg.h

tests/unit/test_atoi_strtou_noneg.c

alejandro-colomar · 2024-01-17T11:13:43Z

v3b changes:

Rebase to master

$ git range-diff master..gh/strtou shadow/master..strtou
1:  e3afc5e9 = 1:  76405163 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
2:  15f03a7c = 2:  11415e3f lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
3:  bdd8f033 = 3:  5fcbea27 tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

alejandro-colomar · 2024-01-17T11:17:31Z

v4 changes:

Drop tests for strtoul(3) and strtoull(3). [@ikerexxe ]

$ git range-diff shadow/master gh/strtou strtou
1:  e3afc5e9 ! 1:  e8a48296 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
    @@ Commit message
         These functions reject negative numbers, instead of silently converting
         them into unsigned, which strtou[l]l(3) do.
     
    +    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Cc: "Serge E. Hallyn" <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/Makefile.am ##
2:  15f03a7c ! 2:  8211b28f lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
    @@ Commit message
         the smallest possible value, 0, and set errno to ERANGE to report an
         error.
     
    +    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Cc: "Serge E. Hallyn" <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## lib/getrange.c ##
3:  bdd8f033 ! 3:  183d3322 tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()
    @@ Metadata
      ## Commit message ##
         tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()
     
    +    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##
    @@ tests/unit/Makefile.am: test_adds_LDADD = \
     
      ## tests/unit/test_atoi_strtou_noneg.c (new) ##
     @@
    -+// SPDX-FileCopyrightText: 2023, Alejandro Colomar <alx@kernel.org>
    ++// SPDX-FileCopyrightText: 2023-2024, Alejandro Colomar <alx@kernel.org>
     +// SPDX-License-Identifier: BSD-3-Clause
     +
     +
    @@ tests/unit/test_atoi_strtou_noneg.c (new)
     +#include "atoi/strtou_noneg.h"
     +
     +
    -+static void test_strtoul(void **state);
     +static void test_strtoul_noneg(void **state);
    -+static void test_strtoull(void **state);
     +static void test_strtoull_noneg(void **state);
     +
     +
    @@ tests/unit/test_atoi_strtou_noneg.c (new)
     +main(void)
     +{
     +    const struct CMUnitTest  tests[] = {
    -+        cmocka_unit_test(test_strtoul),
     +        cmocka_unit_test(test_strtoul_noneg),
    -+        cmocka_unit_test(test_strtoull),
     +        cmocka_unit_test(test_strtoull_noneg),
     +    };
     +
    @@ tests/unit/test_atoi_strtou_noneg.c (new)
     +
     +
     +static void
    -+test_strtoul(void **state)
    -+{
    -+  errno = 0;
    -+  assert_true(strtoul("42", NULL, 0) == 42);
    -+  assert_true(errno == 0);
    -+
    -+  assert_true(strtoul("-1", NULL, 0) == -1ul);
    -+  assert_true(errno == 0);
    -+  assert_true(strtoul("-3", NULL, 0) == -3ul);
    -+  assert_true(errno == 0);
    -+  switch (sizeof(long)) {
    -+  case 8:
    -+          assert_true(strtoul("-0xFFFFFFFFFFFFFFFF", NULL, 0) == 1);
    -+          break;
    -+  case 4:
    -+          assert_true(strtoul("-0xFFFFFFFF", NULL, 0) == 1);
    -+          break;
    -+  }
    -+  assert_true(errno == 0);
    -+
    -+  assert_true(strtoul("-0x10000000000000000", NULL, 0) == ULONG_MAX);
    -+  assert_true(errno == ERANGE);
    -+}
    -+
    -+
    -+static void
     +test_strtoul_noneg(void **state)
     +{
     +  errno = 0;
    @@ tests/unit/test_atoi_strtou_noneg.c (new)
     +
     +
     +static void
    -+test_strtoull(void **state)
    -+{
    -+  errno = 0;
    -+  assert_true(strtoull("42", NULL, 0) == 42);
    -+  assert_true(errno == 0);
    -+
    -+  assert_true(strtoull("-1", NULL, 0) == -1ul);
    -+  assert_true(errno == 0);
    -+  assert_true(strtoull("-3", NULL, 0) == -3ul);
    -+  assert_true(errno == 0);
    -+  assert_true(strtoull("-0xFFFFFFFFFFFFFFFF", NULL, 0) == 1);
    -+  assert_true(errno == 0);
    -+
    -+  assert_true(strtoull("-0x10000000000000000", NULL, 0) == ULONG_MAX);
    -+  assert_true(errno == ERANGE);
    -+}
    -+
    -+
    -+static void
     +test_strtoull_noneg(void **state)
     +{
     +  errno = 0;

alejandro-colomar · 2024-01-17T11:29:51Z

v4b changes:

Rename function parameters

$ git range-diff shadow/master gh/strtou strtou
1:  e8a48296 ! 1:  fda49d88 lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
    @@ lib/atoi/strtou_noneg.c (new)
     +#include "atoi/strtou_noneg.h"
     +
     +
    -+extern inline unsigned long strtoul_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    -+extern inline unsigned long long strtoull_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    ++extern inline unsigned long strtoul_noneg(const char *s,
    ++    char **restrict endp, int base);
    ++extern inline unsigned long long strtoull_noneg(const char *s,
    ++    char **restrict endp, int base);
     
      ## lib/atoi/strtou_noneg.h (new) ##
     @@
    @@ lib/atoi/strtou_noneg.h (new)
     +
     +
     +ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
    -+inline unsigned long strtoul_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    ++inline unsigned long strtoul_noneg(const char *s,
    ++    char **restrict endp, int base);
     +ATTR_STRING(1) ATTR_ACCESS(write_only, 2)
    -+inline unsigned long long strtoull_noneg(const char *nptr,
    -+    char **restrict endptr, int base);
    ++inline unsigned long long strtoull_noneg(const char *s,
    ++    char **restrict endp, int base);
     +
     +
     +inline unsigned long
    -+strtoul_noneg(const char *nptr, char **restrict endptr, int base)
    ++strtoul_noneg(const char *s, char **restrict endp, int base)
     +{
    -+  if (strtol(nptr, endptr, base) < 0) {
    ++  if (strtol(s, endp, base) < 0) {
     +          errno = ERANGE;
     +          return 0;
     +  }
    -+  return strtoul(nptr, endptr, base);
    ++  return strtoul(s, endp, base);
     +}
     +
     +
     +inline unsigned long long
    -+strtoull_noneg(const char *nptr, char **restrict endptr, int base)
    ++strtoull_noneg(const char *s, char **restrict endp, int base)
     +{
    -+  if (strtol(nptr, endptr, base) < 0) {
    ++  if (strtol(s, endp, base) < 0) {
     +          errno = ERANGE;
     +          return 0;
     +  }
    -+  return strtoull(nptr, endptr, base);
    ++  return strtoull(s, endp, base);
     +}
     +
     +
2:  8211b28f = 2:  6cb0d537 lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
3:  183d3322 = 3:  8873245d tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

ikerexxe

LGTM! Since Serge already started reviewing this PR I think it should be him who merges it when it's ready.

These functions reject negative numbers, instead of silently converting them into unsigned, which strtou[l]l(3) do. Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: "Serge E. Hallyn" <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

strtou[l]l(3) silently converts negative numbers into positive. This behavior is wrong: a negative value should be parsed as a negative value, which would underflow unsigned (long) long, and so would return the smallest possible value, 0, and set errno to ERANGE to report an error. Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Cc: "Serge E. Hallyn" <serge@hallyn.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar · 2024-01-17T12:29:26Z

v4c changes:

Add Reviewed-by

$ git range-diff shadow/master gh/strtou strtou
1:  fda49d88 ! 1:  40fe823a lib/atoi/strtou_noneg.[ch]: Add strtou[l]l_noneg()
    @@ Commit message
         These functions reject negative numbers, instead of silently converting
         them into unsigned, which strtou[l]l(3) do.
     
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: "Serge E. Hallyn" <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
2:  6cb0d537 ! 2:  f586e354 lib/, src/: Replace strtou[l]l(3) by strtou[l]l_noneg()
    @@ Commit message
         the smallest possible value, 0, and set errno to ERANGE to report an
         error.
     
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Cc: "Serge E. Hallyn" <serge@hallyn.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
3:  8873245d ! 3:  2fa4837e tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()
    @@ Metadata
      ## Commit message ##
         tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()
     
    -    Cc: Iker Pedrosa <ipedrosa@redhat.com>
    +    Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>
     
      ## tests/unit/Makefile.am ##

alejandro-colomar · 2024-01-17T12:33:24Z

!! This may break some users (already broken users, that is).

We'll need at least a release note.

And I'll remind of this before merging.

hallyn · 2024-01-22T23:17:12Z

Thanks, looks good

hallyn · 2024-01-22T23:19:41Z

!! This may break some users (already broken users, that is).
We'll need at least a release note.

And I'll remind of this before merging.

Is there any case here worth warning about? Certainly not in parsing id mappings.

alejandro-colomar · 2024-01-22T23:46:00Z

!! This may break some users (already broken users, that is).
We'll need at least a release note.

And I'll remind of this before merging.

Is there any case here worth warning about? Certainly not in parsing id mappings.

I'm not sure. :\

Maybe I would just say in the release notes that we're now more strict in what (numeric) input is valid. That would put on alert those who have broken files.

alejandro-colomar mentioned this pull request Dec 20, 2023

lib/getdef.c: Adjust getdef_ boundaries #877

Merged

alejandro-colomar force-pushed the strtou branch 3 times, most recently from 479b590 to c7ecc13 Compare December 29, 2023 21:30

alejandro-colomar marked this pull request as ready for review December 29, 2023 21:35

alejandro-colomar marked this pull request as draft December 30, 2023 12:59

alejandro-colomar force-pushed the strtou branch 2 times, most recently from 6cb388b to 5a30949 Compare January 6, 2024 01:42

alejandro-colomar marked this pull request as ready for review January 6, 2024 01:43

alejandro-colomar requested a review from hallyn January 6, 2024 01:43

alejandro-colomar marked this pull request as draft January 6, 2024 15:06

alejandro-colomar force-pushed the strtou branch from 5a30949 to 1c097ba Compare January 6, 2024 17:01

alejandro-colomar marked this pull request as ready for review January 6, 2024 17:01

alejandro-colomar force-pushed the strtou branch 2 times, most recently from df4deae to 1c097ba Compare January 6, 2024 17:07

alejandro-colomar referenced this pull request Jan 7, 2024

* libmisc/Makefile.am, libmisc/getrange.c: Added function to parse

b8c5483

a range (useful for lastlog).

hallyn reviewed Jan 15, 2024

View reviewed changes

lib/atoi/strtou_noneg.h Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the strtou branch from 1c097ba to 6735732 Compare January 16, 2024 00:38

alejandro-colomar force-pushed the strtou branch from 6735732 to bdd8f03 Compare January 16, 2024 00:42

ikerexxe reviewed Jan 16, 2024

View reviewed changes

lib/atoi/strtou_noneg.h Outdated Show resolved Hide resolved

tests/unit/test_atoi_strtou_noneg.c Outdated Show resolved Hide resolved

alejandro-colomar force-pushed the strtou branch from bdd8f03 to 183d332 Compare January 17, 2024 11:17

alejandro-colomar force-pushed the strtou branch from 183d332 to 8873245 Compare January 17, 2024 11:29

ikerexxe approved these changes Jan 17, 2024

View reviewed changes

alejandro-colomar added 3 commits January 17, 2024 13:28

tests/unit/test_atoi_strtou_noneg.c: Test strtou[l]l_noneg()

2fa4837

Reviewed-by: Iker Pedrosa <ipedrosa@redhat.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>

alejandro-colomar force-pushed the strtou branch from 8873245 to 2fa4837 Compare January 17, 2024 12:29

hallyn merged commit 0138819 into shadow-maint:master Jan 22, 2024
9 checks passed

alejandro-colomar deleted the strtou branch January 22, 2024 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject negative numbers in strtoul(3) #875

Reject negative numbers in strtoul(3) #875

alejandro-colomar commented Dec 17, 2023

alejandro-colomar commented Dec 29, 2023 •

edited

alejandro-colomar commented Dec 30, 2023 •

edited

alejandro-colomar commented Jan 16, 2024

alejandro-colomar commented Jan 16, 2024

alejandro-colomar commented Jan 17, 2024

alejandro-colomar commented Jan 17, 2024

alejandro-colomar commented Jan 17, 2024 •

edited

ikerexxe left a comment

alejandro-colomar commented Jan 17, 2024

alejandro-colomar commented Jan 17, 2024

hallyn commented Jan 22, 2024

hallyn commented Jan 22, 2024

alejandro-colomar commented Jan 22, 2024 •

edited

Reject negative numbers in strtoul(3) #875

Reject negative numbers in strtoul(3) #875

Conversation

alejandro-colomar commented Dec 17, 2023

alejandro-colomar commented Dec 29, 2023 • edited

alejandro-colomar commented Dec 30, 2023 • edited

alejandro-colomar commented Jan 16, 2024

alejandro-colomar commented Jan 16, 2024

alejandro-colomar commented Jan 17, 2024

alejandro-colomar commented Jan 17, 2024

alejandro-colomar commented Jan 17, 2024 • edited

ikerexxe left a comment

Choose a reason for hiding this comment

alejandro-colomar commented Jan 17, 2024

alejandro-colomar commented Jan 17, 2024

hallyn commented Jan 22, 2024

hallyn commented Jan 22, 2024

alejandro-colomar commented Jan 22, 2024 • edited

alejandro-colomar commented Dec 29, 2023 •

edited

alejandro-colomar commented Dec 30, 2023 •

edited

alejandro-colomar commented Jan 17, 2024 •

edited

alejandro-colomar commented Jan 22, 2024 •

edited