Jacob Garber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33787
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
libpayload/libc: Correct strlcat return value
The documented return value for strlcat is horribly wrong, as is the return value itself. It should not return the number of appended bytes, but rather the total size of the concatenated string. From the man page:
The strlcpy() and strlcat() functions return the total length of the string they tried to create. For strlcpy() that means the length of src. For strlcat() that means the initial length of dst plus the length of src. While this may seem somewhat confusing, it was done to make truncation detection simple.
Change-Id: I4421305af85bce88d12d6fdc2eea6807ccdcf449 Signed-off-by: Jacob Garber jgarber1@ualberta.ca --- M payloads/libpayload/libc/string.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/33787/1
diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c index 6c257cb..bd795f4 100644 --- a/payloads/libpayload/libc/string.c +++ b/payloads/libpayload/libc/string.c @@ -249,7 +249,7 @@ * @param d The destination string. * @param s The source string. * @param n d will have at most n-1 characters (plus NUL) after invocation. - * @return A pointer to the destination string. + * @return The total length of the string that would have been created. */ size_t strlcat(char *d, const char *s, size_t n) { @@ -264,7 +264,7 @@ p[i] = s[i];
p[i] = '\0'; - return max; + return sl + dl; }
/**
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33787
to look at the new patch set (#2).
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
libpayload/libc: Correct strlcat return value
The documented return value for strlcat is horribly wrong, as is the return value itself. It should not return the number of appended bytes, but rather the length of the concatenated string. From the man page:
The strlcpy() and strlcat() functions return the total length of the string they tried to create. For strlcpy() that means the length of src. For strlcat() that means the initial length of dst plus the length of src. While this may seem somewhat confusing, it was done to make truncation detection simple.
Change-Id: I4421305af85bce88d12d6fdc2eea6807ccdcf449 Signed-off-by: Jacob Garber jgarber1@ualberta.ca --- M payloads/libpayload/libc/string.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/33787/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33787
to look at the new patch set (#3).
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
libpayload/libc: Correct strlcat return value
The documented return value for strlcat is horribly wrong, as is the return value itself. It should not return the number of appended bytes, but rather the length of the concatenated string. From the man page:
The strlcpy() and strlcat() functions return the total length of the string they tried to create. For strlcpy() that means the length of src. For strlcat() that means the initial length of dst plus the length of src. While this may seem somewhat confusing, it was done to make truncation detection simple.
This change is more likely to fix existing code than break it, since anyone who uses the return value of strlcat will almost certainly rely on the standard behaviour rather than investigate coreboot's source code to see that we have a quirky version.
Change-Id: I4421305af85bce88d12d6fdc2eea6807ccdcf449 Signed-off-by: Jacob Garber jgarber1@ualberta.ca --- M payloads/libpayload/libc/string.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/33787/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33787 )
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c File payloads/libpayload/libc/string.c:
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c@25... PS1, Line 257: int dl = strlen(d); Honestly, this whole implementation is kinda gnarly. We iterate each string twice and in the end I'd say it's still harder to read than a single-pass solution. Maybe we want to replace it completely? Something like
for (i = 0; i < n; i++) if (d[i] == '\0') break;
if (i >= n) return i;
for (; i < n - 1; i++) { d[i] = *s++; if (d[i] == '\0') break; }
d[i] = '\0';
return i;
should work?
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c@26... PS1, Line 267: return sl + dl; This is incorrect if the result was truncated.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33787 )
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c File payloads/libpayload/libc/string.c:
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c@25... PS1, Line 257: int dl = strlen(d);
Honestly, this whole implementation is kinda gnarly. […]
Due to the truncation issue this isn't quite correct. If we're going to replace the whole function then I kinda feel like just copying the one from OpenBSD, which is what Patrick did when he added strlcpy.c
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c@26... PS1, Line 267: return sl + dl;
This is incorrect if the result was truncated.
Sorry, I should have been more clear. The return value should be the length of the concatenated string that would have been created without any truncation.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33787 )
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c File payloads/libpayload/libc/string.c:
https://review.coreboot.org/#/c/33787/1/payloads/libpayload/libc/string.c@26... PS1, Line 267: return sl + dl;
Sorry, I should have been more clear. […]
Oh, looks like I just totally misunderstood the man page. Sorry.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33787 )
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
Patch Set 3:
It seems reasonable that functions wouldn't rely on the broken implementation, but lets wait until after the next release to merge this in case there's something that does expect the old return value.
Jacob Garber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33787 )
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33787/1/payloads/libpayload/libc/st... File payloads/libpayload/libc/string.c:
https://review.coreboot.org/c/coreboot/+/33787/1/payloads/libpayload/libc/st... PS1, Line 257: int dl = strlen(d);
Due to the truncation issue this isn't quite correct. […]
Done
https://review.coreboot.org/c/coreboot/+/33787/1/payloads/libpayload/libc/st... PS1, Line 267: return sl + dl;
Oh, looks like I just totally misunderstood the man page. Sorry.
Done
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33787 )
Change subject: libpayload/libc: Correct strlcat return value ......................................................................
libpayload/libc: Correct strlcat return value
The documented return value for strlcat is horribly wrong, as is the return value itself. It should not return the number of appended bytes, but rather the length of the concatenated string. From the man page:
The strlcpy() and strlcat() functions return the total length of the string they tried to create. For strlcpy() that means the length of src. For strlcat() that means the initial length of dst plus the length of src. While this may seem somewhat confusing, it was done to make truncation detection simple.
This change is more likely to fix existing code than break it, since anyone who uses the return value of strlcat will almost certainly rely on the standard behaviour rather than investigate coreboot's source code to see that we have a quirky version.
Change-Id: I4421305af85bce88d12d6fdc2eea6807ccdcf449 Signed-off-by: Jacob Garber jgarber1@ualberta.ca Reviewed-on: https://review.coreboot.org/c/coreboot/+/33787 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Julius Werner jwerner@chromium.org --- M payloads/libpayload/libc/string.c 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Julius Werner: Looks good to me, approved
diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c index 6c257cb..9a5a1ea 100644 --- a/payloads/libpayload/libc/string.c +++ b/payloads/libpayload/libc/string.c @@ -249,7 +249,7 @@ * @param d The destination string. * @param s The source string. * @param n d will have at most n-1 characters (plus NUL) after invocation. - * @return A pointer to the destination string. + * @return The total length of the concatenated string. */ size_t strlcat(char *d, const char *s, size_t n) { @@ -264,7 +264,7 @@ p[i] = s[i];
p[i] = '\0'; - return max; + return sl + dl; }
/**
-
Jacob Garber (Code Review) -
Julius Werner (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)