Fix string compare functions

Author: padelsbach

.github/workflows/test-units.yml

@@ -10,6 +10,14 @@ jobs:
   unit_tests:
     runs-on: ubuntu-latest
     timeout-minutes: 15
+    strategy:
+      fail-fast: false
+      matrix:
+        mode:
+          - name: normal
+            make_args: ""
+          - name: asan
+            make_args: "ASAN=1"
 
     steps:
       - uses: actions/checkout@v4
@@ -23,10 +31,12 @@ jobs:
         run: |
           make keysclean && make -C tools/keytools clean && rm -f include/target.h
 
-      - name: Build unit tests
+      - name: Build unit tests (${{ matrix.mode.name }})
         run: |
-          make -C tools/unit-tests
+          make -C tools/unit-tests ${{ matrix.mode.make_args }}
 
-      - name: Run unit tests
+      - name: Run unit tests (${{ matrix.mode.name }})
+        env:
+          ASAN_OPTIONS: abort_on_error=1:detect_leaks=1
         run: |
-          make -C tools/unit-tests run
+          make -C tools/unit-tests ${{ matrix.mode.make_args }} run

src/string.c

@@ -105,54 +105,41 @@ char *strcat(char *dest, const char *src)
 
 int strcmp(const char *s1, const char *s2)
 {
-    int diff = 0;
-
-    while (!diff && *s1) {
-        diff = (int)*s1 - (int)*s2;
-        s1++;
-        s2++;
+    while (*s1 && *s2) {
+        int c1 = ((unsigned char)*s1++);
+        int c2 = ((unsigned char)*s2++);
+        if (c1 != c2)
+            return c1 - c2;
     }
-
-    return diff;
+    return ((unsigned char)*s1) - ((unsigned char)*s2);
 }
 #endif /* Renesas CCRX */
 
 int strcasecmp(const char *s1, const char *s2)
 {
-    int diff = 0;
-
-    while (!diff && *s1) {
-        diff = (int)*s1 - (int)*s2;
-
-        if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
-                (isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
-            diff = 0;
-
-        s1++;
-        s2++;
+    while (*s1 && *s2) {
+        int c1 = tolower((unsigned char)*s1++);
+        int c2 = tolower((unsigned char)*s2++);
+        if (c1 != c2)
+            return c1 - c2;
     }
-
-    return diff;
+    return tolower((unsigned char)*s1) - tolower((unsigned char)*s2);
 }
 
 int strncasecmp(const char *s1, const char *s2, size_t n)
 {
-    int diff = 0;
-    size_t i = 0;
-
-    while (!diff && *s1) {
-        diff = (int)*s1 - (int)*s2;
+    if (n == 0)
+        return 0;
 
-        if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
-                (isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
-            diff = 0;
-
-        s1++;
-        s2++;
-        if (++i >= n)
-            break;
+    while (n--) {
+        int c1 = tolower((unsigned char)*s1++);
+        int c2 = tolower((unsigned char)*s2++);
+        if (c1 != c2)
+            return c1 - c2;
+        if (c1 == '\0')
+            return 0;
     }
-    return diff;
+    return 0;
 }
 
 #if !defined(__CCRX__) /* Renesas CCRX */
@@ -202,7 +189,7 @@ char *strcpy(char *dst, const char *src)
 {
    size_t i = 0;
 
-    while(1) {
+    while (1) {
         dst[i] = src[i];
         if (src[i] == '\0')
             break;

tools/unit-tests/Makefile

@@ -22,6 +22,12 @@ CFLAGS+=-DUNIT_TEST -DWOLFSSL_USER_SETTINGS
 LDFLAGS+=-fprofile-arcs
 LDFLAGS+=-ftest-coverage
 
+ASAN?=0
+ifeq ($(ASAN),1)
+	CFLAGS+=-fsanitize=address -fno-omit-frame-pointer -O1
+	LDFLAGS+=-fsanitize=address
+endif
+
 
 
 
@@ -33,6 +39,12 @@ TESTS:=unit-parser unit-extflash unit-string unit-spi-flash unit-aes128 \
 
 all: $(TESTS)
 
+asan:
+	$(MAKE) ASAN=1 all
+
+asan-run:
+	$(MAKE) ASAN=1 run
+
 cov:
 	gcovr -f "^\.\.\/\.\.\/src.*\.c" -r ../.. --verbose \
 		--merge-mode-functions merge-use-line-0 \
@@ -73,6 +85,7 @@ unit-update-flash:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH
 unit-update-ram:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH \
 	-DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT \
 	-DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT -DNO_XIP
+unit-string:CFLAGS+=-fno-builtin
 
 
 WOLFCRYPT_CFLAGS+=-DWOLFBOOT_SIGN_ECC256 -DWOLFBOOT_SIGN_ECC256 -DHAVE_ECC_KEY_IMPORT -D__WOLFBOOT
@@ -175,4 +188,4 @@ covclean:
 clean: covclean
 	rm -f $(TESTS) *.o *.gcno *.gcda coverage.*
 
-.PHONY: FORCE
+.PHONY: FORCE asan asan-run

tools/unit-tests/unit-string.c

@@ -92,6 +92,24 @@ START_TEST(test_case_insensitive_alpha_only)
 }
 END_TEST
 
+START_TEST(test_strcasecmp_prefix_regression)
+{
+    ck_assert_int_lt(strcasecmp("a", "ab"), 0);
+    ck_assert_int_gt(strcasecmp("ab", "a"), 0);
+    ck_assert_int_lt(strcasecmp("", "a"), 0);
+    ck_assert_int_gt(strcasecmp("a", ""), 0);
+}
+END_TEST
+
+START_TEST(test_strncasecmp_n_limit_regression)
+{
+    ck_assert_int_eq(strncasecmp("ABC", "abc", 0), 0);
+    ck_assert_int_eq(strncasecmp("", "a", 0), 0);
+    ck_assert_int_eq(strncasecmp("AbCd", "aBcE", 3), 0);
+    ck_assert_int_lt(strncasecmp("AbCd", "aBcE", 4), 0);
+}
+END_TEST
+
 START_TEST(test_isalpha_helpers)
 {
     ck_assert_int_eq(islower('a'), 1);
@@ -145,6 +163,16 @@ START_TEST(test_strlen_strcmp)
 }
 END_TEST
 
+START_TEST(test_strcmp_prefix_termination)
+{
+    ck_assert_int_lt(strcmp("a", "abc"), 0);
+    ck_assert_int_lt(strcmp("ab", "abc"), 0);
+    ck_assert_int_gt(strcmp("abc", "ab"), 0);
+    ck_assert_int_gt(strcmp("abc", "a"), 0);
+    ck_assert_int_eq(strcmp("", ""), 0);
+}
+END_TEST
+
 START_TEST(test_strcpy_strncpy_strcat_strncat)
 {
     char buf[8];
@@ -329,9 +357,12 @@ Suite *string_suite(void)
     tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_exact);
     tcase_add_test(tcase_strncasecmp, test_strncasecmp_diff_before_n);
     tcase_add_test(tcase_strncasecmp, test_case_insensitive_alpha_only);
+    tcase_add_test(tcase_strncasecmp, test_strcasecmp_prefix_regression);
+    tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_limit_regression);
     tcase_add_test(tcase_misc, test_isalpha_helpers);
     tcase_add_test(tcase_misc, test_memset_memcmp_memchr);
     tcase_add_test(tcase_misc, test_strlen_strcmp);
+    tcase_add_test(tcase_misc, test_strcmp_prefix_termination);
     tcase_add_test(tcase_misc, test_strcpy_strncpy_strcat_strncat);
     tcase_add_test(tcase_misc, test_strncmp);
     tcase_add_test(tcase_misc, test_memcpy_memmove);