Hello Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32029
to review the following change.
Change subject: libpayload: strtoull: Fix edge case bug with *endptr ......................................................................
libpayload: strtoull: Fix edge case bug with *endptr
strtoull() can optionally take a second pointer as an out-parameter that will be adjusted to point to the end of the parsed string. This works almost right, but misses one important edge case: when the parsed string is "0", the function will interpret the leading '0' as an octal prefix, so that the first actually parsed digit is already the terminating '\0' byte. This will cause the function to early abort, which still (correctly) returns 0 but doesn't adjust *endptr.
The early abort is pointless anyway -- the only other thing the function does is run a for-loop whose condition is the exact inverse (so it's guaranteed to run zero iterations in this case) and then adjust *endptr (which we want). So just take it out. This also technically corrects the behavior of *endptr for a completely invalid string, since the strtoull man page says
If there were no digits at all, strtoul() stores the original value of nptr in *endptr (and returns 0).
Change-Id: Idddd74e18e410a9d0b6dce9512ca0412b9e2333c Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/libc/string.c 1 file changed, 0 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/32029/1
diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c index a58efde..2266d4b 100644 --- a/payloads/libpayload/libc/string.c +++ b/payloads/libpayload/libc/string.c @@ -521,12 +521,6 @@ 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);
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32029 )
Change subject: libpayload: strtoull: Fix edge case bug with *endptr ......................................................................
Patch Set 1:
"0x" seems to be the opposite case and needs the bail-out, afaics
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32029 )
Change subject: libpayload: strtoull: Fix edge case bug with *endptr ......................................................................
Patch Set 1:
"0x" seems to be the opposite case and needs the bail-out, afaics
Oh, you mean if there's only "0x" without another valid digit after it? I think in that case it's actually supposed to parse the '0' and set *endptr to the 'x'... at least that seems to be what glibc is doing. I can fix that too.
Hello build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32029
to look at the new patch set (#2).
Change subject: libpayload: strtoull: Fix edge case bug with *endptr ......................................................................
libpayload: strtoull: Fix edge case bug with *endptr
strtoull() can optionally take a second pointer as an out-parameter that will be adjusted to point to the end of the parsed string. This works almost right, but misses two important edge cases: firstly,when the parsed string is "0", the function will interpret the leading '0' as an octal prefix, so that the first actually parsed digit is already the terminating '\0' byte. This will cause the function to early abort, which still (correctly) returns 0 but doesn't adjust *endptr.
The early abort is pointless anyway -- the only other thing the function does is run a for-loop whose condition is the exact inverse (so it's guaranteed to run zero iterations in this case) and then adjust *endptr (which we want). So just take it out. This also technically corrects the behavior of *endptr for a completely invalid string, since the strtoull man page says
If there were no digits at all, strtoul() stores the original value of nptr in *endptr (and returns 0).
The second issue occurs when the parsed string is "0x" without another valid digit behind it. In this case, we will still jump over the 0x prefix so that *endptr is set to the first byte after that. The correct interpretation in this case is that there is no 0x prefix, and instead a valid 0 digit with the 'x' being invalid garbage at the end. By not skipping the prefix unless there's at least one valid digit after it, we get the correct behavior of *endptr pointing to the 'x'.
Change-Id: Idddd74e18e410a9d0b6dce9512ca0412b9e2333c Signed-off-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/libc/string.c 1 file changed, 2 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/32029/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32029 )
Change subject: libpayload: strtoull: Fix edge case bug with *endptr ......................................................................
Patch Set 2: Code-Review+2
"0x" seems to be the opposite case and needs the bail-out, afaics
Oh, you mean if there's only "0x" without another valid digit after it? I think in that case it's actually supposed to parse the '0' and set *endptr to the 'x'... at least that seems to be what glibc is doing. I can fix that too.
Right. I only saw that you are changing this case too and overlooked that it was broken anyway. Thanks for fixing it!
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32029 )
Change subject: libpayload: strtoull: Fix edge case bug with *endptr ......................................................................
Patch Set 2: Code-Review+1
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32029 )
Change subject: libpayload: strtoull: Fix edge case bug with *endptr ......................................................................
libpayload: strtoull: Fix edge case bug with *endptr
strtoull() can optionally take a second pointer as an out-parameter that will be adjusted to point to the end of the parsed string. This works almost right, but misses two important edge cases: firstly,when the parsed string is "0", the function will interpret the leading '0' as an octal prefix, so that the first actually parsed digit is already the terminating '\0' byte. This will cause the function to early abort, which still (correctly) returns 0 but doesn't adjust *endptr.
The early abort is pointless anyway -- the only other thing the function does is run a for-loop whose condition is the exact inverse (so it's guaranteed to run zero iterations in this case) and then adjust *endptr (which we want). So just take it out. This also technically corrects the behavior of *endptr for a completely invalid string, since the strtoull man page says
If there were no digits at all, strtoul() stores the original value of nptr in *endptr (and returns 0).
The second issue occurs when the parsed string is "0x" without another valid digit behind it. In this case, we will still jump over the 0x prefix so that *endptr is set to the first byte after that. The correct interpretation in this case is that there is no 0x prefix, and instead a valid 0 digit with the 'x' being invalid garbage at the end. By not skipping the prefix unless there's at least one valid digit after it, we get the correct behavior of *endptr pointing to the 'x'.
Change-Id: Idddd74e18e410a9d0b6dce9512ca0412b9e2333c Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32029 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M payloads/libpayload/libc/string.c 1 file changed, 2 insertions(+), 7 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c index a58efde..f563f63 100644 --- a/payloads/libpayload/libc/string.c +++ b/payloads/libpayload/libc/string.c @@ -517,16 +517,11 @@ /* Base 16 allows the 0x on front - so skip over it */
if (base == 16) { - if (ptr[0] == '0' && (ptr[1] == 'x' || ptr[1] == 'X')) + if (ptr[0] == '0' && (ptr[1] == 'x' || ptr[1] == 'X') && + _valid(ptr[2], base)) 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);
-
Julius Werner (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)