From 8097809c8aa976d425b0d75459f1e9c06c39e73d Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Fri, 8 Aug 2025 19:34:48 -0700 Subject: [PATCH] libpayload: Fix strsep() edge cases Our strsep() function is slightly incorrect in that it leaves the `stringp` pointer pointing to the terminating NUL byte after parsing the last token. The man page for official implementations says: > In case no delimiter was found, the token is taken to be the entire > string *stringp, and *stringp is made NULL. This doesn't affect things in practice much because we also (incorrectly) return NULL when called with `**stringp == '\0'`, meaning the usual pattern of calling `strsep()` in a row without checking results first still works when there are less tokens than expected, since we terminate early from that case instead. But it does break the edge cases where the caller wants to check if there were extra bytes beyond the last token (`stringp == NULL`), and where we call `strsep()` on a pointer pointing directly to a terminating NUL byte already (supposed to return an empty string but our implementation actually returns NULL). It doesn't look like these edge cases occur anywhere in current libpayload or depthcharge code. This patch fixes the issue and also adds a unit test to ensure it remains correct in the future. (Also move the definition of the `errno` variable from lib.c into string.c, because `perror()` in string.c is the only function that actually needs that, and the crazy linker error you get when only linking one but not the other into a test will waste you half an hour to figure out.) Change-Id: I610b5117710c110bcba4fac2a0bb6c13f4f8d046 Signed-off-by: Julius Werner Reviewed-on: https://review.coreboot.org/c/coreboot/+/88729 Reviewed-by: Yu-Ping Wu Tested-by: build bot (Jenkins) --- payloads/libpayload/libc/lib.c | 2 - payloads/libpayload/libc/string.c | 13 +++++-- payloads/libpayload/tests/libc/Makefile.mk | 5 ++- payloads/libpayload/tests/libc/string-test.c | 39 ++++++++++++++++++++ 4 files changed, 52 insertions(+), 7 deletions(-) create mode 100644 payloads/libpayload/tests/libc/string-test.c diff --git a/payloads/libpayload/libc/lib.c b/payloads/libpayload/libc/lib.c index f1d85745ad..0475789c00 100644 --- a/payloads/libpayload/libc/lib.c +++ b/payloads/libpayload/libc/lib.c @@ -124,8 +124,6 @@ void __noreturn abort(void) halt(); } -int errno; - char *getenv(const char *name) { return NULL; diff --git a/payloads/libpayload/libc/string.c b/payloads/libpayload/libc/string.c index a1b7d4d59d..d641431e7d 100644 --- a/payloads/libpayload/libc/string.c +++ b/payloads/libpayload/libc/string.c @@ -290,7 +290,7 @@ char *strsep(char **stringp, const char *delim) { char *walk, *token; - if (!stringp || !*stringp || !**stringp) + if (!stringp || !*stringp) return NULL; token = walk = *stringp; @@ -302,11 +302,12 @@ char *strsep(char **stringp, const char *delim) if (*walk) { /* NUL terminate */ *walk = '\0'; - walk++; + *stringp = walk + 1; + } else { + /* Set to NULL after last token. */ + *stringp = NULL; } - *stringp = walk; - return token; } @@ -547,6 +548,10 @@ char *strtok(char *str, const char *delim) return strtok_r(str, delim, &strtok_ptr); } +/* errno isn't actually used anywhere other than perror() below, and just here + for compatibility with libc-targeting code wanting to incidentally print it. */ +int errno; + /** * Print error message and error number * @param s Error message to print diff --git a/payloads/libpayload/tests/libc/Makefile.mk b/payloads/libpayload/tests/libc/Makefile.mk index 5f92bdf0a8..8d88af9f30 100644 --- a/payloads/libpayload/tests/libc/Makefile.mk +++ b/payloads/libpayload/tests/libc/Makefile.mk @@ -1,3 +1,6 @@ -tests-y += fmap_locate_area-test +tests-y += fmap_locate_area-test string-test fmap_locate_area-test-srcs += tests/libc/fmap_locate_area-test.c + +string-test-srcs += tests/libc/string-test.c +string-test-srcs += libc/string.c diff --git a/payloads/libpayload/tests/libc/string-test.c b/payloads/libpayload/tests/libc/string-test.c new file mode 100644 index 0000000000..2fe7c6bace --- /dev/null +++ b/payloads/libpayload/tests/libc/string-test.c @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include +#include + +static void test_strsep(void **state) +{ + /* Use `char x[] = ` instead of `char *x = ` to avoid writing to .rodata. */ + char test1[] = "abc,def|ghi jklm,\no"; + const char *delim = ", \n"; + + char *stringp = test1; + assert_string_equal(strsep(&stringp, delim), "abc"); + assert_ptr_equal(stringp, test1 + 4); + assert_string_equal(strsep(&stringp, delim), "def|ghi"); + assert_ptr_equal(stringp, test1 + 12); + assert_string_equal(strsep(&stringp, delim), "jklm"); + assert_ptr_equal(stringp, test1 + 17); + assert_string_equal(strsep(&stringp, delim), ""); + assert_ptr_equal(stringp, test1 + 18); + assert_string_equal(strsep(&stringp, delim), "o"); + assert_null(stringp); + assert_null(strsep(&stringp, delim)); + assert_null(stringp); + + char test2[] = ""; + stringp = test2; + assert_string_equal(strsep(&stringp, delim), ""); + assert_null(strsep(&stringp, delim)); +} + +int main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_strsep), + }; + + return lp_run_group_tests(tests, NULL, NULL); +}