From ddd22b2f533db9c0da0bb262fbafa51f67c8587e Mon Sep 17 00:00:00 2001 From: jvoisin Date: Fri, 1 May 2026 00:36:32 +0200 Subject: Fix strncat/wcsncat Previously, no checks were done when __n <= __b, but strncat _appends_ after existing content, making this a overly broad check check. For example, with an 8-byte buffer containing "12345\0", strncat(buf, "ABCD", 4) would have the check skipped, but the result "12345ABCD\0" is 10 bytes, resulting in an overflow. This commit fixes this oversight, and adds a bunch of tests. --- tests/Makefile | 10 ++++++++++ tests/test_mbsnrtowcs_dynamic.c | 4 +--- tests/test_strncat_dynamic_write.c | 2 -- tests/test_strncat_n_eq_buf.c | 18 ++++++++++++++++++ tests/test_strncat_n_gt_buf.c | 17 +++++++++++++++++ tests/test_strncat_n_lt_buf.c | 18 ++++++++++++++++++ tests/test_strncat_n_one.c | 18 ++++++++++++++++++ tests/test_strncat_safe.c | 34 ++++++++++++++++++++++++++++++++++ tests/test_strncat_static_write.c | 10 +++++----- tests/test_wcsncat_n_eq_buf.c | 18 ++++++++++++++++++ tests/test_wcsncat_n_gt_buf.c | 17 +++++++++++++++++ tests/test_wcsncat_n_lt_buf.c | 18 ++++++++++++++++++ tests/test_wcsncat_n_one.c | 18 ++++++++++++++++++ tests/test_wcsncat_safe.c | 34 ++++++++++++++++++++++++++++++++++ tests/test_wcsnrtombs_dynamic.c | 4 +--- 15 files changed, 227 insertions(+), 13 deletions(-) create mode 100644 tests/test_strncat_n_eq_buf.c create mode 100644 tests/test_strncat_n_gt_buf.c create mode 100644 tests/test_strncat_n_lt_buf.c create mode 100644 tests/test_strncat_n_one.c create mode 100644 tests/test_strncat_safe.c create mode 100644 tests/test_wcsncat_n_eq_buf.c create mode 100644 tests/test_wcsncat_n_gt_buf.c create mode 100644 tests/test_wcsncat_n_lt_buf.c create mode 100644 tests/test_wcsncat_n_one.c create mode 100644 tests/test_wcsncat_safe.c (limited to 'tests') diff --git a/tests/Makefile b/tests/Makefile index 6904b2d..9bedd16 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -84,6 +84,11 @@ RUNTIME_TARGETS= \ test_strlcpy_dynamic_write \ test_strlcpy_static_write \ test_strncat_dynamic_write \ + test_strncat_n_eq_buf \ + test_strncat_n_gt_buf \ + test_strncat_n_lt_buf \ + test_strncat_n_one \ + test_strncat_safe \ test_strncat_static_write \ test_strncpy_dynamic_write \ test_strncpy_static_write \ @@ -101,6 +106,11 @@ RUNTIME_TARGETS= \ test_wcsnrtombs_static \ test_wcscat_static_write \ test_wcscpy_static_write \ + test_wcsncat_n_eq_buf \ + test_wcsncat_n_gt_buf \ + test_wcsncat_n_lt_buf \ + test_wcsncat_n_one \ + test_wcsncat_safe \ test_wcsncat_static_write \ test_wcsncpy_static_write \ test_wmemcpy_dynamic_read \ diff --git a/tests/test_mbsnrtowcs_dynamic.c b/tests/test_mbsnrtowcs_dynamic.c index 77b9082..58575d3 100644 --- a/tests/test_mbsnrtowcs_dynamic.c +++ b/tests/test_mbsnrtowcs_dynamic.c @@ -14,9 +14,7 @@ int main(int argc, char** argv) { srcp = src; mbsnrtowcs(buffer, &srcp, 2, 2, &st); - /* Unsafe: ask to write argc (10) wide chars into 4-element buffer. - * Before the fix, the else branch clamped source bytes instead of - * the output wide-char count, allowing destination overflow. */ + /* Unsafe: ask to write argc (10) wide chars into 4-element buffer. */ CHK_FAIL_START srcp = src; memset(&st, 0, sizeof(st)); diff --git a/tests/test_strncat_dynamic_write.c b/tests/test_strncat_dynamic_write.c index d5c5a94..c538339 100644 --- a/tests/test_strncat_dynamic_write.c +++ b/tests/test_strncat_dynamic_write.c @@ -7,11 +7,9 @@ int main(int argc, char** argv) { strncat(buffer, "1234567", 5); puts(buffer); -#if 0 CHK_FAIL_START strncat(buffer, argv[1], argc); CHK_FAIL_END -#endif puts(buffer); return ret; diff --git a/tests/test_strncat_n_eq_buf.c b/tests/test_strncat_n_eq_buf.c new file mode 100644 index 0000000..bbc39d2 --- /dev/null +++ b/tests/test_strncat_n_eq_buf.c @@ -0,0 +1,18 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + char buffer[8] = {0}; + strncat(buffer, "12345", 5); + puts(buffer); + + /* n == buffer size but overflow due to existing content. + * buffer has 5 chars, src "ABC" (len 3): 5+3+1 = 9 > 8 → overflow. */ + CHK_FAIL_START + strncat(buffer, "ABC", 8); + CHK_FAIL_END + + puts(buffer); + return ret; +} diff --git a/tests/test_strncat_n_gt_buf.c b/tests/test_strncat_n_gt_buf.c new file mode 100644 index 0000000..cca226b --- /dev/null +++ b/tests/test_strncat_n_gt_buf.c @@ -0,0 +1,17 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + char buffer[8] = {0}; + strncat(buffer, "12345", 5); + puts(buffer); + + /* n > buffer size and overflow: n=10, buffer has 5 chars. */ + CHK_FAIL_START + strncat(buffer, "1234567890", 10); + CHK_FAIL_END + + puts(buffer); + return ret; +} diff --git a/tests/test_strncat_n_lt_buf.c b/tests/test_strncat_n_lt_buf.c new file mode 100644 index 0000000..35a0ba1 --- /dev/null +++ b/tests/test_strncat_n_lt_buf.c @@ -0,0 +1,18 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + char buffer[8] = {0}; + strncat(buffer, "12345", 5); + puts(buffer); + + /* n < buffer size but overflow due to existing content. + * buffer has 5 chars, src "ABCD" (len 4), min(4,3)=3: 5+3+1 = 9 > 8. */ + CHK_FAIL_START + strncat(buffer, "ABCD", 3); + CHK_FAIL_END + + puts(buffer); + return ret; +} diff --git a/tests/test_strncat_n_one.c b/tests/test_strncat_n_one.c new file mode 100644 index 0000000..fce46bd --- /dev/null +++ b/tests/test_strncat_n_one.c @@ -0,0 +1,18 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + char buffer[8] = {0}; + strncat(buffer, "1234567", 7); + puts(buffer); + + /* n=1, buffer has 7 chars ("1234567"). + * 7+1+1 = 9 > 8 → overflow. Even n=1 can overflow a nearly-full buffer. */ + CHK_FAIL_START + strncat(buffer, "X", 1); + CHK_FAIL_END + + puts(buffer); + return ret; +} diff --git a/tests/test_strncat_safe.c b/tests/test_strncat_safe.c new file mode 100644 index 0000000..ff8cadd --- /dev/null +++ b/tests/test_strncat_safe.c @@ -0,0 +1,34 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + char buffer[8] = {0}; + + /* Safe: empty buffer, append 7 chars with n=7 → "1234567\0" = exactly 8 bytes */ + strncat(buffer, "1234567", 7); + puts(buffer); + + /* Safe: reset and append with n larger than source. + * src is "AB" (len 2), n=100 → only 2 chars copied + NUL = 3 bytes */ + buffer[0] = '\0'; + strncat(buffer, "AB", 100); + puts(buffer); + + /* Safe: partially filled, append fits exactly. + * buffer = "ABCD" (4 chars), append "EFG" with n=3 → 4+3+1 = 8 = exact fit */ + buffer[0] = '\0'; + strncat(buffer, "ABCD", 4); + strncat(buffer, "EFG", 3); + puts(buffer); + + /* Safe: n limits copy to fit. + * buffer = "ABCDE" (5 chars), src = "FGHIJKLM" (8 chars), n=2 → 5+2+1 = 8 */ + buffer[0] = '\0'; + strncat(buffer, "ABCDE", 5); + strncat(buffer, "FGHIJKLM", 2); + puts(buffer); + + /* All safe operations passed without trapping */ + return 0; +} diff --git a/tests/test_strncat_static_write.c b/tests/test_strncat_static_write.c index 7fe89ff..53d1532 100644 --- a/tests/test_strncat_static_write.c +++ b/tests/test_strncat_static_write.c @@ -4,15 +4,15 @@ int main(int argc, char** argv) { char buffer[8] = {0}; - char src[] = {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0'}; - strncat(buffer, src, 5); + strncat(buffer, "12345", 5); puts(buffer); -#if 0 + /* n=4 is less than buffer size (8), but buffer already has 5 chars, + * so appending 4 more + NUL = 10 bytes total, overflowing the buffer. + */ CHK_FAIL_START - strncat(buffer, src, 10); + strncat(buffer, "ABCD", 4); CHK_FAIL_END -#endif puts(buffer); return ret; diff --git a/tests/test_wcsncat_n_eq_buf.c b/tests/test_wcsncat_n_eq_buf.c new file mode 100644 index 0000000..e516842 --- /dev/null +++ b/tests/test_wcsncat_n_eq_buf.c @@ -0,0 +1,18 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + wchar_t buffer[8] = {0}; + wcsncat(buffer, L"12345", 5); + printf("%ls\n", buffer); + + /* n == buffer capacity but overflow due to existing content. + * buffer has 5 wchars, src L"ABC" (len 3): 5+3+1 = 9 > 8 → overflow. */ + CHK_FAIL_START + wcsncat(buffer, L"ABC", 8); + CHK_FAIL_END + + printf("%ls\n", buffer); + return ret; +} diff --git a/tests/test_wcsncat_n_gt_buf.c b/tests/test_wcsncat_n_gt_buf.c new file mode 100644 index 0000000..186b8c0 --- /dev/null +++ b/tests/test_wcsncat_n_gt_buf.c @@ -0,0 +1,17 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + wchar_t buffer[8] = {0}; + wcsncat(buffer, L"12345", 5); + printf("%ls\n", buffer); + + /* n > buffer capacity and overflow: n=10, buffer has 5 wchars. */ + CHK_FAIL_START + wcsncat(buffer, L"1234567890", 10); + CHK_FAIL_END + + printf("%ls\n", buffer); + return ret; +} diff --git a/tests/test_wcsncat_n_lt_buf.c b/tests/test_wcsncat_n_lt_buf.c new file mode 100644 index 0000000..99a42de --- /dev/null +++ b/tests/test_wcsncat_n_lt_buf.c @@ -0,0 +1,18 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + wchar_t buffer[8] = {0}; + wcsncat(buffer, L"12345", 5); + printf("%ls\n", buffer); + + /* n < buffer capacity but overflow due to existing content. + * buffer has 5 wchars, src L"ABCD" (len 4), min(4,3)=3: 5+3+1 = 9 > 8. */ + CHK_FAIL_START + wcsncat(buffer, L"ABCD", 3); + CHK_FAIL_END + + printf("%ls\n", buffer); + return ret; +} diff --git a/tests/test_wcsncat_n_one.c b/tests/test_wcsncat_n_one.c new file mode 100644 index 0000000..23de28e --- /dev/null +++ b/tests/test_wcsncat_n_one.c @@ -0,0 +1,18 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + wchar_t buffer[8] = {0}; + wcsncat(buffer, L"1234567", 7); + printf("%ls\n", buffer); + + /* n=1, buffer has 7 wchars. + * 7+1+1 = 9 > 8 → overflow. Even n=1 can overflow a nearly-full buffer. */ + CHK_FAIL_START + wcsncat(buffer, L"X", 1); + CHK_FAIL_END + + printf("%ls\n", buffer); + return ret; +} diff --git a/tests/test_wcsncat_safe.c b/tests/test_wcsncat_safe.c new file mode 100644 index 0000000..00fc8e6 --- /dev/null +++ b/tests/test_wcsncat_safe.c @@ -0,0 +1,34 @@ +#include "common.h" + +#include + +int main(int argc, char** argv) { + wchar_t buffer[8] = {0}; + + /* Safe: empty buffer, append 7 wide chars → exactly fills buffer */ + wcsncat(buffer, L"ABCDEFG", 7); + printf("%ls\n", buffer); + + /* Safe: reset, append with n larger than source. + * src is L"AB" (len 2), n=100 → only 2 wchars copied */ + buffer[0] = L'\0'; + wcsncat(buffer, L"AB", 100); + printf("%ls\n", buffer); + + /* Safe: partially filled, append fits exactly. + * buffer = L"ABCD" (4 wchars), append L"EFG" n=3 → 4+3+1 = 8 = exact fit */ + buffer[0] = L'\0'; + wcsncat(buffer, L"ABCD", 4); + wcsncat(buffer, L"EFG", 3); + printf("%ls\n", buffer); + + /* Safe: n limits copy to fit. + * buffer = L"ABCDE" (5 wchars), src long, n=2 → 5+2+1 = 8 = exact fit */ + buffer[0] = L'\0'; + wcsncat(buffer, L"ABCDE", 5); + wcsncat(buffer, L"FGHIJKLM", 2); + printf("%ls\n", buffer); + + /* All safe operations passed without trapping */ + return 0; +} diff --git a/tests/test_wcsnrtombs_dynamic.c b/tests/test_wcsnrtombs_dynamic.c index 808c9c8..28b03bf 100644 --- a/tests/test_wcsnrtombs_dynamic.c +++ b/tests/test_wcsnrtombs_dynamic.c @@ -14,9 +14,7 @@ int main(int argc, char** argv) { srcp = src; wcsnrtombs(buffer, &srcp, 4, 4, &st); - /* Unsafe: ask to write argc (10) bytes into 8-byte buffer. - * Before the fix, the first branch incorrectly divided the byte-sized - * buffer capacity by sizeof(wchar_t), making the check too permissive. */ + /* Unsafe: ask to write argc (10) bytes into 8-byte buffer. */ CHK_FAIL_START srcp = src; memset(&st, 0, sizeof(st)); -- cgit v1.3