Skip to content

Commit 5a7f133

Browse files
padelsbachpadelsbach
committed
Fix string compare functions
1 parent 4a53bd1 commit 5a7f133

File tree

7 files changed

+174
-58
lines changed

7 files changed

+174
-58
lines changed

‎.gitignore‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ tools/unit-tests/unit-aes128
140140
tools/unit-tests/unit-aes256
141141
tools/unit-tests/unit-chacha20
142142
tools/unit-tests/unit-delta
143+
tools/unit-tests/unit-disk
143144
tools/unit-tests/unit-enc-nvm
144145
tools/unit-tests/unit-enc-nvm-flagshome
145146
tools/unit-tests/unit-extflash
@@ -153,7 +154,9 @@ tools/unit-tests/unit-pci
153154
tools/unit-tests/unit-pkcs11_store
154155
tools/unit-tests/unit-sectorflags
155156
tools/unit-tests/unit-spi-flash
157+
tools/unit-tests/unit-string
156158
tools/unit-tests/unit-update-flash
159+
tools/unit-tests/unit-update-flash-enc
157160
tools/unit-tests/unit-update-ram
158161

159162

‎lib/wolfssl‎

Submodule wolfssl updated 1534 files

‎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
@@ -109,54 +109,41 @@ char *strcat(char *dest, const char *src)
109109

110110
int strcmp(const char *s1, const char *s2)
111111
{
112-
int diff = 0;
113-
114-
while (!diff && *s1) {
115-
diff = (int)*s1 - (int)*s2;
116-
s1++;
117-
s2++;
112+
while (*s1 && *s2) {
113+
int c1 = ((unsigned char)*s1++);
114+
int c2 = ((unsigned char)*s2++);
115+
if (c1 != c2)
116+
return c1 - c2;
118117
}
119-
120-
return diff;
118+
return ((unsigned char)*s1) - ((unsigned char)*s2);
121119
}
122120
#endif /* Renesas CCRX */
123121

124122
int strcasecmp(const char *s1, const char *s2)
125123
{
126-
int diff = 0;
127-
128-
while (!diff && *s1) {
129-
diff = (int)*s1 - (int)*s2;
130-
131-
if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
132-
(isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
133-
diff = 0;
134-
135-
s1++;
136-
s2++;
124+
while (*s1 && *s2) {
125+
int c1 = tolower((unsigned char)*s1++);
126+
int c2 = tolower((unsigned char)*s2++);
127+
if (c1 != c2)
128+
return c1 - c2;
137129
}
138-
139-
return diff;
130+
return tolower((unsigned char)*s1) - tolower((unsigned char)*s2);
140131
}
141132

142133
int strncasecmp(const char *s1, const char *s2, size_t n)
143134
{
144-
int diff = 0;
145-
size_t i = 0;
146-
147-
while (!diff && *s1) {
148-
diff = (int)*s1 - (int)*s2;
135+
if (n == 0)
136+
return 0;
149137

150-
if (((diff == 'A' - 'a') || (diff == 'a' - 'A')) &&
151-
(isalpha((unsigned char)*s1) && isalpha((unsigned char)*s2)))
152-
diff = 0;
153-
154-
s1++;
155-
s2++;
156-
if (++i >= n)
157-
break;
138+
while (n--) {
139+
int c1 = tolower((unsigned char)*s1++);
140+
int c2 = tolower((unsigned char)*s2++);
141+
if (c1 != c2)
142+
return c1 - c2;
143+
if (c1 == '\0')
144+
return 0;
158145
}
159-
return diff;
146+
return 0;
160147
}
161148

162149
#if !defined(__CCRX__) /* Renesas CCRX */
@@ -206,7 +193,7 @@ char *strcpy(char *dst, const char *src)
206193
{
207194
size_t i = 0;
208195

209-
while(1) {
196+
while (1) {
210197
dst[i] = src[i];
211198
if (src[i] == '\0')
212199
break;

‎tools/test.mk‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1110,11 +1110,11 @@ test-all: clean
11101110
test-size-all:
11111111
make test-size SIGN=NONE LIMIT=5040 NO_ARM_ASM=1
11121112
make keysclean
1113-
make test-size SIGN=ED25519 LIMIT=11700 NO_ARM_ASM=1
1113+
make test-size SIGN=ED25519 LIMIT=11724 NO_ARM_ASM=1
11141114
make keysclean
11151115
make test-size SIGN=ECC256 LIMIT=18944 NO_ARM_ASM=1
11161116
make clean
1117-
make test-size SIGN=ECC256 NO_ASM=1 LIMIT=13840 NO_ARM_ASM=1
1117+
make test-size SIGN=ECC256 NO_ASM=1 LIMIT=13856 NO_ARM_ASM=1
11181118
make keysclean
11191119
make test-size SIGN=RSA2048 LIMIT=11916 NO_ARM_ASM=1
11201120
make clean
@@ -1126,21 +1126,21 @@ test-size-all:
11261126
make keysclean
11271127
make test-size SIGN=ECC384 LIMIT=19888 NO_ARM_ASM=1
11281128
make clean
1129-
make test-size SIGN=ECC384 NO_ASM=1 LIMIT=15216 NO_ARM_ASM=1
1129+
make test-size SIGN=ECC384 NO_ASM=1 LIMIT=15232 NO_ARM_ASM=1
11301130
make keysclean
1131-
make test-size SIGN=ED448 LIMIT=13760 NO_ARM_ASM=1
1131+
make test-size SIGN=ED448 LIMIT=13776 NO_ARM_ASM=1
11321132
make keysclean
11331133
make test-size SIGN=RSA3072 LIMIT=12056 NO_ARM_ASM=1
11341134
make clean
11351135
make test-size SIGN=RSA3072 NO_ASM=1 LIMIT=12600 NO_ARM_ASM=1
11361136
make keysclean
11371137
make test-size SIGN=LMS LMS_LEVELS=2 LMS_HEIGHT=5 LMS_WINTERNITZ=8 \
11381138
WOLFBOOT_SMALL_STACK=0 IMAGE_SIGNATURE_SIZE=2644 \
1139-
IMAGE_HEADER_SIZE?=5288 LIMIT=7696 NO_ARM_ASM=1
1139+
IMAGE_HEADER_SIZE?=5288 LIMIT=7712 NO_ARM_ASM=1
11401140
make keysclean
11411141
make test-size SIGN=XMSS XMSS_PARAMS='XMSS-SHA2_10_256' \
11421142
IMAGE_SIGNATURE_SIZE=2500 IMAGE_HEADER_SIZE?=4096 \
1143-
LIMIT=8560 NO_ARM_ASM=1
1143+
LIMIT=8568 NO_ARM_ASM=1
11441144
make keysclean
11451145
make clean
11461146
make test-size SIGN=ML_DSA ML_DSA_LEVEL=2 LIMIT=19362 \

‎tools/unit-tests/Makefile‎

Lines changed: 7 additions & 0 deletions
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

@@ -74,6 +80,7 @@ unit-update-flash:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH
7480
unit-update-ram:CFLAGS+=-DMOCK_PARTITIONS -DWOLFBOOT_NO_SIGN -DUNIT_TEST_AUTH \
7581
-DWOLFBOOT_HASH_SHA256 -DPRINTF_ENABLED -DEXT_FLASH -DPART_UPDATE_EXT \
7682
-DPART_SWAP_EXT -DPART_BOOT_EXT -DWOLFBOOT_DUALBOOT -DNO_XIP
83+
unit-string:CFLAGS+=-fno-builtin
7784

7885

7986
WOLFCRYPT_CFLAGS+=-DWOLFBOOT_SIGN_ECC256 -DWOLFBOOT_SIGN_ECC256 -DHAVE_ECC_KEY_IMPORT -D__WOLFBOOT

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

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

95+
START_TEST(test_strcasecmp_mixed_alnum_punct)
96+
{
97+
ck_assert_int_eq(strcasecmp("Boot-123_OK!", "bOot-123_ok!"), 0);
98+
ck_assert_int_eq(strcasecmp("v1.2.3-rc1", "V1.2.3-RC1"), 0);
99+
ck_assert_int_eq(strcasecmp("A_B-C.D", "a_b-c.d"), 0);
100+
}
101+
END_TEST
102+
103+
START_TEST(test_strcasecmp_non_alpha_ordering)
104+
{
105+
ck_assert_int_lt(strcasecmp("abc-1", "abc_1"), 0);
106+
ck_assert_int_gt(strcasecmp("abc_1", "abc-1"), 0);
107+
ck_assert_int_lt(strcasecmp("abc1", "abc2"), 0);
108+
ck_assert_int_gt(strcasecmp("abc2", "abc1"), 0);
109+
}
110+
END_TEST
111+
112+
START_TEST(test_strncasecmp_non_alpha_n_boundaries)
113+
{
114+
ck_assert_int_eq(strncasecmp("Boot-123_OK!", "bOot-123_ok?", 11), 0);
115+
ck_assert_int_lt(strncasecmp("Boot-123_OK!", "bOot-123_ok?", 12), 0);
116+
ck_assert_int_gt(strncasecmp("bOot-123_ok?", "Boot-123_OK!", 12), 0);
117+
ck_assert_int_eq(strncasecmp("A1.B2-C3", "a1.b2-c3", 8), 0);
118+
}
119+
END_TEST
120+
121+
START_TEST(test_strcasecmp_prefix_regression)
122+
{
123+
ck_assert_int_lt(strcasecmp("a", "ab"), 0);
124+
ck_assert_int_gt(strcasecmp("ab", "a"), 0);
125+
ck_assert_int_lt(strcasecmp("", "a"), 0);
126+
ck_assert_int_gt(strcasecmp("a", ""), 0);
127+
}
128+
END_TEST
129+
130+
START_TEST(test_strncasecmp_n_limit_regression)
131+
{
132+
ck_assert_int_eq(strncasecmp("ABC", "abc", 0), 0);
133+
ck_assert_int_eq(strncasecmp("", "a", 0), 0);
134+
ck_assert_int_eq(strncasecmp("AbCd", "aBcE", 3), 0);
135+
ck_assert_int_lt(strncasecmp("AbCd", "aBcE", 4), 0);
136+
}
137+
END_TEST
138+
139+
START_TEST(test_strncasecmp_stop_at_null_regression)
140+
{
141+
const char s1[] = { 'A', '\0', 'x', '\0' };
142+
const char s2[] = { 'a', '\0', 'Y', '\0' };
143+
144+
ck_assert_int_eq(strncasecmp(s1, s2, 2), 0);
145+
ck_assert_int_eq(strncasecmp(s1, s2, 3), 0);
146+
ck_assert_int_eq(strncasecmp(s1, s2, 8), 0);
147+
}
148+
END_TEST
149+
150+
START_TEST(test_strncasecmp_prefix_large_n_regression)
151+
{
152+
ck_assert_int_lt(strncasecmp("a", "ab", 8), 0);
153+
ck_assert_int_gt(strncasecmp("ab", "a", 8), 0);
154+
ck_assert_int_lt(strncasecmp("A", "aB", 8), 0);
155+
}
156+
END_TEST
157+
158+
START_TEST(test_strncasecmp_no_read_past_null_when_n_remaining)
159+
{
160+
const char s1[] = { '\0' };
161+
const char s2[] = { '\0' };
162+
163+
/*
164+
* Regression target: if implementation does not stop on '\0' when n > 1,
165+
* the next loop iteration reads past both 1-byte buffers.
166+
*/
167+
ck_assert_int_eq(strncasecmp(s1, s2, 2), 0);
168+
ck_assert_int_eq(strncasecmp(s1, s2, 8), 0);
169+
}
170+
END_TEST
171+
95172
START_TEST(test_isalpha_helpers)
96173
{
97174
ck_assert_int_eq(islower('a'), 1);
@@ -145,6 +222,16 @@ START_TEST(test_strlen_strcmp)
145222
}
146223
END_TEST
147224

225+
START_TEST(test_strcmp_prefix_termination)
226+
{
227+
ck_assert_int_lt(strcmp("a", "abc"), 0);
228+
ck_assert_int_lt(strcmp("ab", "abc"), 0);
229+
ck_assert_int_gt(strcmp("abc", "ab"), 0);
230+
ck_assert_int_gt(strcmp("abc", "a"), 0);
231+
ck_assert_int_eq(strcmp("", ""), 0);
232+
}
233+
END_TEST
234+
148235
START_TEST(test_strcpy_strncpy_strcat_strncat)
149236
{
150237
char buf[8];
@@ -329,9 +416,18 @@ Suite *string_suite(void)
329416
tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_exact);
330417
tcase_add_test(tcase_strncasecmp, test_strncasecmp_diff_before_n);
331418
tcase_add_test(tcase_strncasecmp, test_case_insensitive_alpha_only);
419+
tcase_add_test(tcase_strncasecmp, test_strcasecmp_mixed_alnum_punct);
420+
tcase_add_test(tcase_strncasecmp, test_strcasecmp_non_alpha_ordering);
421+
tcase_add_test(tcase_strncasecmp, test_strncasecmp_non_alpha_n_boundaries);
422+
tcase_add_test(tcase_strncasecmp, test_strcasecmp_prefix_regression);
423+
tcase_add_test(tcase_strncasecmp, test_strncasecmp_n_limit_regression);
424+
tcase_add_test(tcase_strncasecmp, test_strncasecmp_stop_at_null_regression);
425+
tcase_add_test(tcase_strncasecmp, test_strncasecmp_prefix_large_n_regression);
426+
tcase_add_test(tcase_strncasecmp, test_strncasecmp_no_read_past_null_when_n_remaining);
332427
tcase_add_test(tcase_misc, test_isalpha_helpers);
333428
tcase_add_test(tcase_misc, test_memset_memcmp_memchr);
334429
tcase_add_test(tcase_misc, test_strlen_strcmp);
430+
tcase_add_test(tcase_misc, test_strcmp_prefix_termination);
335431
tcase_add_test(tcase_misc, test_strcpy_strncpy_strcat_strncat);
336432
tcase_add_test(tcase_misc, test_strncmp);
337433
tcase_add_test(tcase_misc, test_memcpy_memmove);

0 commit comments

Comments
 (0)