Skip to content

Commit bc846fa

Browse files
padelsbachpadelsbach
committed
Fix string compare functions
1 parent 5075db7 commit bc846fa

9 files changed

Lines changed: 124 additions & 60 deletions

File tree

‎.github/workflows/test-units.yml‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,14 @@ jobs:
1010
unit_tests:
1111
runs-on: ubuntu-latest
1212
timeout-minutes: 15
13+
strategy:
14+
fail-fast: false
15+
matrix:
16+
mode:
17+
- name: normal
18+
make_args: ""
19+
- name: asan
20+
make_args: "ASAN=1"
1321

1422
steps:
1523
- uses: actions/checkout@v4
@@ -23,10 +31,12 @@ jobs:
2331
run: |
2432
make keysclean && make -C tools/keytools clean && rm -f include/target.h
2533
26-
- name: Build unit tests
34+
- name: Build unit tests (${{ matrix.mode.name }})
2735
run: |
28-
make -C tools/unit-tests
36+
make -C tools/unit-tests ${{ matrix.mode.make_args }}
2937
30-
- name: Run unit tests
38+
- name: Run unit tests (${{ matrix.mode.name }})
39+
env:
40+
ASAN_OPTIONS: abort_on_error=1:detect_leaks=1
3141
run: |
32-
make -C tools/unit-tests run
42+
make -C tools/unit-tests ${{ matrix.mode.make_args }} run

‎src/libwolfboot.c‎

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -861,51 +861,74 @@ void RAMFUNCTION wolfBoot_success(void)
861861
*/
862862
uint16_t wolfBoot_find_header(uint8_t *haystack, uint16_t type, uint8_t **ptr)
863863
{
864-
uint8_t *p = haystack;
864+
uint8_t *p;
865865
uint16_t len, htype;
866-
const volatile uint8_t *max_p = (haystack - IMAGE_HEADER_OFFSET) +
867-
IMAGE_HEADER_SIZE;
866+
uintptr_t p_addr, max_addr;
867+
868868
*ptr = NULL;
869-
if (p > max_p) {
869+
870+
if (haystack == NULL) {
871+
unit_dbg("Illegal address (NULL)\n");
872+
return 0;
873+
}
874+
875+
p_addr = (uintptr_t)haystack;
876+
if (p_addr < IMAGE_HEADER_OFFSET) {
877+
unit_dbg("Illegal address (too low)\n");
878+
return 0;
879+
}
880+
881+
max_addr = p_addr - IMAGE_HEADER_OFFSET;
882+
if (max_addr > (UINTPTR_MAX - IMAGE_HEADER_SIZE)) {
883+
unit_dbg("Illegal address (overflow)\n");
884+
return 0;
885+
}
886+
max_addr += IMAGE_HEADER_SIZE;
887+
888+
if (p_addr > max_addr) {
870889
unit_dbg("Illegal address (too high)\n");
871890
return 0;
872891
}
873-
while ((p + 4) < max_p) {
892+
893+
while (p_addr < max_addr) {
894+
if ((max_addr - p_addr) < 4U) {
895+
break;
896+
}
897+
p = (uint8_t *)p_addr;
874898
htype = p[0] | (p[1] << 8);
875899
if (htype == 0) {
876900
unit_dbg("Explicit end of options reached\n");
877901
break;
878902
}
879903
/* skip unaligned half-words and padding bytes */
880-
if ((p[0] == HDR_PADDING) || ((((size_t)p) & 0x01) != 0)) {
881-
p++;
904+
if ((p[0] == HDR_PADDING) || ((p_addr & 0x01U) != 0U)) {
905+
p_addr++;
882906
continue;
883907
}
884908

885909
len = p[2] | (p[3] << 8);
886910
/* check len */
887-
if ((4 + len) > (uint16_t)(IMAGE_HEADER_SIZE - IMAGE_HEADER_OFFSET)) {
911+
if ((4U + len) > (uint16_t)(IMAGE_HEADER_SIZE - IMAGE_HEADER_OFFSET)) {
888912
unit_dbg("This field is too large (bigger than the space available "
889913
"in the current header)\n");
890-
unit_dbg("%d %d %d\n", len, IMAGE_HEADER_SIZE, IMAGE_HEADER_OFFSET);
914+
unit_dbg("%u %u %u\n", (unsigned int)len,
915+
(unsigned int)IMAGE_HEADER_SIZE,
916+
(unsigned int)IMAGE_HEADER_OFFSET);
891917
break;
892918
}
893919
/* check max pointer */
894-
if (p + 4 + len > max_p) {
920+
if ((max_addr - p_addr) < (uintptr_t)(4U + len)) {
895921
unit_dbg("This field is too large and would overflow the image "
896922
"header\n");
897923
break;
898924
}
899925

900-
/* skip header [type|len] */
901-
p += 4;
902-
903926
if (htype == type) {
904927
/* found, return pointer to data portion */
905-
*ptr = p;
928+
*ptr = (uint8_t *)(p_addr + 4U);
906929
return len;
907930
}
908-
p += len;
931+
p_addr += (uintptr_t)(4U + len);
909932
}
910933
return 0;
911934
}

‎src/string.c‎

Lines changed: 23 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -105,54 +105,41 @@ char *strcat(char *dest, const char *src)
105105

106106
int strcmp(const char *s1, const char *s2)
107107
{
108-
int diff = 0;
109-
110-
while (!diff && *s1) {
111-
diff = (int)*s1 - (int)*s2;
112-
s1++;
113-
s2++;
108+
while (*s1 && *s2) {
109+
int c1 = ((unsigned char)*s1++);
110+
int c2 = ((unsigned char)*s2++);
111+
if (c1 != c2)
112+
return c1 - c2;
114113
}
115-
116-
return diff;
114+
return ((unsigned char)*s1) - ((unsigned char)*s2);
117115
}
118116
#endif /* Renesas CCRX */
119117

120118
int strcasecmp(const char *s1, const char *s2)
121119
{
122-
int diff = 0;
123-
124-
while (!diff && *s1) {
125-
diff = (int)*s1 - (int)*s2;
126-
127-
if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
128-
(isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
129-
diff = 0;
130-
131-
s1++;
132-
s2++;
120+
while (*s1 && *s2) {
121+
int c1 = tolower((unsigned char)*s1++);
122+
int c2 = tolower((unsigned char)*s2++);
123+
if (c1 != c2)
124+
return c1 - c2;
133125
}
134-
135-
return diff;
126+
return tolower((unsigned char)*s1) - tolower((unsigned char)*s2);
136127
}
137128

138129
int strncasecmp(const char *s1, const char *s2, size_t n)
139130
{
140-
int diff = 0;
141-
size_t i = 0;
142-
143-
while (!diff && *s1) {
144-
diff = (int)*s1 - (int)*s2;
131+
if (n == 0)
132+
return 0;
145133

146-
if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
147-
(isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
148-
diff = 0;
149-
150-
s1++;
151-
s2++;
152-
if (++i >= n)
153-
break;
134+
while (n--) {
135+
int c1 = tolower((unsigned char)*s1++);
136+
int c2 = tolower((unsigned char)*s2++);
137+
if (c1 != c2)
138+
return c1 - c2;
139+
if (c1 == '\0')
140+
return 0;
154141
}
155-
return diff;
142+
return 0;
156143
}
157144

158145
#if !defined(__CCRX__) /* Renesas CCRX */
@@ -202,7 +189,7 @@ char *strcpy(char *dst, const char *src)
202189
{
203190
size_t i = 0;
204191

205-
while(1) {
192+
while (1) {
206193
dst[i] = src[i];
207194
if (src[i] == '\0')
208195
break;

‎tools/unit-tests/Makefile‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,12 @@ CFLAGS+=-DUNIT_TEST -DWOLFSSL_USER_SETTINGS
2222
LDFLAGS+=-fprofile-arcs
2323
LDFLAGS+=-ftest-coverage
2424

25+
ASAN?=0
26+
ifeq ($(ASAN),1)
27+
CFLAGS+=-fsanitize=address -fno-omit-frame-pointer -O1
28+
LDFLAGS+=-fsanitize=address
29+
endif
30+
2531

2632

2733

@@ -33,6 +39,12 @@ TESTS:=unit-parser unit-extflash unit-string unit-spi-flash unit-aes128 \
3339

3440
all: $(TESTS)
3541

42+
asan:
43+
$(MAKE) ASAN=1 all
44+
45+
asan-run:
46+
$(MAKE) ASAN=1 run
47+
3648
cov:
3749
gcovr -f "^\.\.\/\.\.\/src.*\.c" -r ../.. --verbose \
3850
--merge-mode-functions merge-use-line-0 \
@@ -73,6 +85,7 @@ unit-update-flash:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH
7385
unit-update-ram:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH \
7486
-DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT \
7587
-DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT -DNO_XIP
88+
unit-string:CFLAGS+=-fno-builtin
7689

7790

7891
WOLFCRYPT_CFLAGS+=-DWOLFBOOT_SIGN_ECC256 -DWOLFBOOT_SIGN_ECC256 -DHAVE_ECC_KEY_IMPORT -D__WOLFBOOT
@@ -175,4 +188,4 @@ covclean:
175188
clean: covclean
176189
rm -f $(TESTS) *.o *.gcno *.gcda coverage.*
177190

178-
.PHONY: FORCE
191+
.PHONY: FORCE asan asan-run

‎tools/unit-tests/unit-disk‎

346 KB
Binary file not shown.

‎tools/unit-tests/unit-image.c‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ int wc_ecc_verify_hash_ex(mp_int *r, mp_int *s, const byte* hash,
341341

342342
START_TEST(test_verify_signature)
343343
{
344-
uint8_t pubkey[32];
344+
uint8_t sig[ECC_IMAGE_SIGNATURE_SIZE] = {0};
345345
struct wolfBoot_image test_img;
346346

347347
test_img.part = PART_UPDATE;
@@ -352,13 +352,13 @@ START_TEST(test_verify_signature)
352352
ck_assert_int_eq(verify_called, 0);
353353

354354
ecc_init_fail = 1;
355-
wolfBoot_verify_signature_ecc(0, NULL, pubkey);
355+
wolfBoot_verify_signature_ecc(0, NULL, sig);
356356
ck_assert_int_eq(verify_called, 0);
357357

358358
ecc_init_fail = 0;
359359
verify_called = 0;
360360
ecc_import_fail = 1;
361-
wolfBoot_verify_signature_ecc(0, NULL, pubkey);
361+
wolfBoot_verify_signature_ecc(0, NULL, sig);
362362
ck_assert_int_eq(verify_called, 0);
363363

364364
ecc_init_fail = 0;
@@ -368,7 +368,7 @@ START_TEST(test_verify_signature)
368368
ext_flash_erase(0, 2 * WOLFBOOT_SECTOR_SIZE);
369369
ext_flash_write(0, test_img_v200000000_signed_bin,
370370
test_img_len);
371-
wolfBoot_verify_signature_ecc(0, &test_img, pubkey);
371+
wolfBoot_verify_signature_ecc(0, &test_img, sig);
372372
ck_assert_int_eq(verify_called, 1);
373373
}
374374
END_TEST

‎tools/unit-tests/unit-string‎

326 KB
Binary file not shown.

‎tools/unit-tests/unit-string.c‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,24 @@ START_TEST(test_case_insensitive_alpha_only)
9292
}
9393
END_TEST
9494

95+
START_TEST(test_strcasecmp_prefix_regression)
96+
{
97+
ck_assert_int_lt(strcasecmp("a", "ab"), 0);
98+
ck_assert_int_gt(strcasecmp("ab", "a"), 0);
99+
ck_assert_int_lt(strcasecmp("", "a"), 0);
100+
ck_assert_int_gt(strcasecmp("a", ""), 0);
101+
}
102+
END_TEST
103+
104+
START_TEST(test_strncasecmp_n_limit_regression)
105+
{
106+
ck_assert_int_eq(strncasecmp("ABC", "abc", 0), 0);
107+
ck_assert_int_eq(strncasecmp("", "a", 0), 0);
108+
ck_assert_int_eq(strncasecmp("AbCd", "aBcE", 3), 0);
109+
ck_assert_int_lt(strncasecmp("AbCd", "aBcE", 4), 0);
110+
}
111+
END_TEST
112+
95113
START_TEST(test_isalpha_helpers)
96114
{
97115
ck_assert_int_eq(islower('a'), 1);
@@ -145,6 +163,16 @@ START_TEST(test_strlen_strcmp)
145163
}
146164
END_TEST
147165

166+
START_TEST(test_strcmp_prefix_termination)
167+
{
168+
ck_assert_int_lt(strcmp("a", "abc"), 0);
169+
ck_assert_int_lt(strcmp("ab", "abc"), 0);
170+
ck_assert_int_gt(strcmp("abc", "ab"), 0);
171+
ck_assert_int_gt(strcmp("abc", "a"), 0);
172+
ck_assert_int_eq(strcmp("", ""), 0);
173+
}
174+
END_TEST
175+
148176
START_TEST(test_strcpy_strncpy_strcat_strncat)
149177
{
150178
char buf[8];
@@ -329,9 +357,12 @@ Suite *string_suite(void)
329357
tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_exact);
330358
tcase_add_test(tcase_strncasecmp, test_strncasecmp_diff_before_n);
331359
tcase_add_test(tcase_strncasecmp, test_case_insensitive_alpha_only);
360+
tcase_add_test(tcase_strncasecmp, test_strcasecmp_prefix_regression);
361+
tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_limit_regression);
332362
tcase_add_test(tcase_misc, test_isalpha_helpers);
333363
tcase_add_test(tcase_misc, test_memset_memcmp_memchr);
334364
tcase_add_test(tcase_misc, test_strlen_strcmp);
365+
tcase_add_test(tcase_misc, test_strcmp_prefix_termination);
335366
tcase_add_test(tcase_misc, test_strcpy_strncpy_strcat_strncat);
336367
tcase_add_test(tcase_misc, test_strncmp);
337368
tcase_add_test(tcase_misc, test_memcpy_memmove);

‎tools/unit-tests/unit-update-flash-enc‎

413 KB
Binary file not shown.

0 commit comments

Comments
 (0)