Hello Nico Huber,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32086
to review the following change.
Change subject: libpayload: Deduplicate strtol and strtoull ......................................................................
libpayload: Deduplicate strtol and strtoull
Our strtol() and strtoull() function contain almost exactly the same code. This is a) bad in general and b) may cause the code to get out of sync, such as it recently happened with CB:32029.
This patch changes strtol() to be based on strtoull() so that the main parsing code exists only once, and also adds a strtoll() to round off the library. Also fix the bounds imposed by strtoul() to be based on the actual length of a 'long', not hardcoded to 32-bits (which is not equivalent on all architectures).
Change-Id: I919c65a773cecdb11739c3f22dd0d182ed50c07f Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/include/stdlib.h M payloads/libpayload/libc/string.c 2 files changed, 26 insertions(+), 40 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/86/32086/1
diff --git a/payloads/libpayload/include/stdlib.h b/payloads/libpayload/include/stdlib.h index aaacc87..d4a7844 100644 --- a/payloads/libpayload/include/stdlib.h +++ b/payloads/libpayload/include/stdlib.h @@ -196,6 +196,7 @@ * @{ */ long int strtol(const char *s, char **nptr, int base); +long long int strtoll(const char *s, char **nptr, int base); unsigned long int strtoul(const char *s, char **nptr, int base); unsigned long long int strtoull(const char *s, char **nptr, int base); long atol(const char *nptr); diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c index f563f63..6c257cb 100644 --- a/payloads/libpayload/libc/string.c +++ b/payloads/libpayload/libc/string.c @@ -33,6 +33,7 @@ #include <string.h> #include <ctype.h> #include <inttypes.h> +#include <limits.h> #include <errno.h>
/** @@ -419,59 +420,43 @@ * @return A signed integer representation of the string */
-long int strtol(const char *ptr, char **endptr, int base) +long long int strtoll(const char *orig_ptr, char **endptr, int base) { - int ret = 0; - int negative = 1; - - if (endptr != NULL) - *endptr = (char *) ptr; + const char *ptr = orig_ptr; + int is_negative = 0;
/* Purge whitespace */
for( ; *ptr && isspace(*ptr); ptr++);
if (ptr[0] == '-') { - negative = -1; + is_negative = 1; ptr++; }
- if (!*ptr) - return 0; + unsigned long long uval = strtoull(ptr, endptr, base);
- /* Determine the base */ + /* If the whole string is unparseable, endptr should point to start. */ + if (endptr && *endptr == ptr) + *endptr = (char *)orig_ptr;
- if (base == 0) { - if (ptr[0] == '0' && (ptr[1] == 'x' || ptr[1] == 'X')) - base = 16; - else if (ptr[0] == '0') { - base = 8; - ptr++; - } - else - base = 10; - } + if (uval > (unsigned long long)LLONG_MAX + !!is_negative) + uval = (unsigned long long)LLONG_MAX + !!is_negative;
- /* Base 16 allows the 0x on front - so skip over it */ + if (is_negative) + return -uval; + else + return uval; +}
- if (base == 16) { - if (ptr[0] == '0' && (ptr[1] == 'x' || ptr[1] == 'X')) - ptr += 2; - } - - /* If the first character isn't valid, then don't - * bother */ - - if (!*ptr || !_valid(*ptr, base)) - return 0; - - for( ; *ptr && _valid(*ptr, base); ptr++) - ret = (ret * base) + _offset(*ptr, base); - - if (endptr != NULL) - *endptr = (char *) ptr; - - return ret * negative; +long int strtol(const char *ptr, char **endptr, int base) +{ + long long int val = strtoll(ptr, endptr, base); + if (val > LONG_MAX) + return LONG_MAX; + if (val < LONG_MIN) + return LONG_MIN; + return val; }
long atol(const char *nptr) @@ -534,7 +519,7 @@ unsigned long int strtoul(const char *ptr, char **endptr, int base) { unsigned long long val = strtoull(ptr, endptr, base); - if (val > UINT32_MAX) return UINT32_MAX; + if (val > ULONG_MAX) return ULONG_MAX; return val; }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32086 )
Change subject: libpayload: Deduplicate strtol and strtoull ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32086/1/payloads/libpayload/libc/string.c File payloads/libpayload/libc/string.c:
https://review.coreboot.org/#/c/32086/1/payloads/libpayload/libc/string.c@52... PS1, Line 522: if (val > ULONG_MAX) return ULONG_MAX; trailing statements should be on next line
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32086 )
Change subject: libpayload: Deduplicate strtol and strtoull ......................................................................
Patch Set 1:
Ran a couple of tests to make sure it still spits out the right values. For this I hacked the strto(u)l() bounds to use INT_MAX/MIN instead of LONG_MAX/MIN, so the difference is visible on arm64 (because normally the l and ll functions always show the same):
TEST: strtoll(0) == 0 TEST: strtol(0) == 0 TEST: strtoull(0) == 0 TEST: strtoul(0) == 0 TEST: strtoll(0x7fffffff) == 2147483647 TEST: strtol(0x7fffffff) == 2147483647 TEST: strtoull(0x7fffffff) == 2147483647 TEST: strtoul(0x7fffffff) == 2147483647 TEST: strtoll(0x80000000) == 2147483648 TEST: strtol(0x80000000) == 2147483647 TEST: strtoull(0x80000000) == 2147483648 TEST: strtoul(0x80000000) == 2147483648 TEST: strtoll(0xffffffff) == 4294967295 TEST: strtol(0xffffffff) == 2147483647 TEST: strtoull(0xffffffff) == 4294967295 TEST: strtoul(0xffffffff) == 4294967295 TEST: strtoll(0x100000000LL) == 4294967296 TEST: strtol(0x100000000LL) == 2147483647 TEST: strtoull(0x100000000LL) == 4294967296 TEST: strtoul(0x100000000LL) == 4294967295 TEST: strtoll(0x7fffffffffffffffLL) == 9223372036854775807 TEST: strtol(0x7fffffffffffffffLL) == 2147483647 TEST: strtoull(0x7fffffffffffffffLL) == 9223372036854775807 TEST: strtoul(0x7fffffffffffffffLL) == 4294967295 TEST: strtoll(0x8000000000000000ULL) == 9223372036854775807 TEST: strtol(0x8000000000000000ULL) == 2147483647 TEST: strtoull(0x8000000000000000ULL) == 9223372036854775808 TEST: strtoul(0x8000000000000000ULL) == 4294967295 TEST: strtoll(0xffffffffffffffffULL) == 9223372036854775807 TEST: strtol(0xffffffffffffffffULL) == 2147483647 TEST: strtoull(0xffffffffffffffffULL) == 18446744073709551615 TEST: strtoul(0xffffffffffffffffULL) == 4294967295 TEST: strtoll(-0x7fffffff) == -2147483647 TEST: strtol(-0x7fffffff) == -2147483647 TEST: strtoll(-0x80000000) == -2147483648 TEST: strtol(-0x80000000) == -2147483648 TEST: strtoll(-0x80000001) == -2147483649 TEST: strtol(-0x80000001) == -2147483648 TEST: strtoll(-0xffffffff) == -4294967295 TEST: strtol(-0xffffffff) == -2147483648 TEST: strtoll(-0x100000000LL) == -4294967296 TEST: strtol(-0x100000000LL) == -2147483648 TEST: strtoll(-0x7fffffffffffffffLL) == -9223372036854775807 TEST: strtol(-0x7fffffffffffffffLL) == -2147483648 TEST: strtoll(-0x8000000000000000) == -9223372036854775808 TEST: strtol(-0x8000000000000000) == -2147483648 TEST: strtoll(-0x8000000000000001) == -9223372036854775808 TEST: strtol(-0x8000000000000001) == -2147483648
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32086 )
Change subject: libpayload: Deduplicate strtol and strtoull ......................................................................
Patch Set 1:
*ping*
Any takers?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32086 )
Change subject: libpayload: Deduplicate strtol and strtoull ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32086 )
Change subject: libpayload: Deduplicate strtol and strtoull ......................................................................
libpayload: Deduplicate strtol and strtoull
Our strtol() and strtoull() function contain almost exactly the same code. This is a) bad in general and b) may cause the code to get out of sync, such as it recently happened with CB:32029.
This patch changes strtol() to be based on strtoull() so that the main parsing code exists only once, and also adds a strtoll() to round off the library. Also fix the bounds imposed by strtoul() to be based on the actual length of a 'long', not hardcoded to 32-bits (which is not equivalent on all architectures).
Change-Id: I919c65a773cecdb11739c3f22dd0d182ed50c07f Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32086 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M payloads/libpayload/include/stdlib.h M payloads/libpayload/libc/string.c 2 files changed, 26 insertions(+), 40 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/payloads/libpayload/include/stdlib.h b/payloads/libpayload/include/stdlib.h index 3f94d21..06fb735 100644 --- a/payloads/libpayload/include/stdlib.h +++ b/payloads/libpayload/include/stdlib.h @@ -186,6 +186,7 @@ * @{ */ long int strtol(const char *s, char **nptr, int base); +long long int strtoll(const char *s, char **nptr, int base); unsigned long int strtoul(const char *s, char **nptr, int base); unsigned long long int strtoull(const char *s, char **nptr, int base); long atol(const char *nptr); diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c index f563f63..6c257cb 100644 --- a/payloads/libpayload/libc/string.c +++ b/payloads/libpayload/libc/string.c @@ -33,6 +33,7 @@ #include <string.h> #include <ctype.h> #include <inttypes.h> +#include <limits.h> #include <errno.h>
/** @@ -419,59 +420,43 @@ * @return A signed integer representation of the string */
-long int strtol(const char *ptr, char **endptr, int base) +long long int strtoll(const char *orig_ptr, char **endptr, int base) { - int ret = 0; - int negative = 1; - - if (endptr != NULL) - *endptr = (char *) ptr; + const char *ptr = orig_ptr; + int is_negative = 0;
/* Purge whitespace */
for( ; *ptr && isspace(*ptr); ptr++);
if (ptr[0] == '-') { - negative = -1; + is_negative = 1; ptr++; }
- if (!*ptr) - return 0; + unsigned long long uval = strtoull(ptr, endptr, base);
- /* Determine the base */ + /* If the whole string is unparseable, endptr should point to start. */ + if (endptr && *endptr == ptr) + *endptr = (char *)orig_ptr;
- if (base == 0) { - if (ptr[0] == '0' && (ptr[1] == 'x' || ptr[1] == 'X')) - base = 16; - else if (ptr[0] == '0') { - base = 8; - ptr++; - } - else - base = 10; - } + if (uval > (unsigned long long)LLONG_MAX + !!is_negative) + uval = (unsigned long long)LLONG_MAX + !!is_negative;
- /* Base 16 allows the 0x on front - so skip over it */ + if (is_negative) + return -uval; + else + return uval; +}
- if (base == 16) { - if (ptr[0] == '0' && (ptr[1] == 'x' || ptr[1] == 'X')) - ptr += 2; - } - - /* If the first character isn't valid, then don't - * bother */ - - if (!*ptr || !_valid(*ptr, base)) - return 0; - - for( ; *ptr && _valid(*ptr, base); ptr++) - ret = (ret * base) + _offset(*ptr, base); - - if (endptr != NULL) - *endptr = (char *) ptr; - - return ret * negative; +long int strtol(const char *ptr, char **endptr, int base) +{ + long long int val = strtoll(ptr, endptr, base); + if (val > LONG_MAX) + return LONG_MAX; + if (val < LONG_MIN) + return LONG_MIN; + return val; }
long atol(const char *nptr) @@ -534,7 +519,7 @@ unsigned long int strtoul(const char *ptr, char **endptr, int base) { unsigned long long val = strtoull(ptr, endptr, base); - if (val > UINT32_MAX) return UINT32_MAX; + if (val > ULONG_MAX) return ULONG_MAX; return val; }
-
build bot (Jenkins) (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)