Skip to content

Fix string compare functions#707

Merged
danielinux merged 1 commit intowolfSSL:masterfrom
padelsbach:padelsbach/finding-219
Mar 6, 2026
Merged

Fix string compare functions#707
danielinux merged 1 commit intowolfSSL:masterfrom
padelsbach:padelsbach/finding-219

Conversation

@padelsbach
Copy link
Contributor

Fixes finding 219.

  • Update string compare functions to avoid false equality
  • Add regression test cases
  • Ensure -fno-builtin is set for the unit-string test to ensure we test our versions of the string functions
  • Add asan mode to test Makefile and CI

@padelsbach padelsbach force-pushed the padelsbach/finding-219 branch 8 times, most recently from 70cfff9 to 6f3e2a0 Compare March 3, 2026 22:26
@padelsbach padelsbach marked this pull request as ready for review March 5, 2026 07:54
@danielinux danielinux self-assigned this Mar 5, 2026
@danielinux danielinux requested review from Copilot and danielinux March 5, 2026 09:01
Copy link
Member

@danielinux danielinux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a safety improvement in general. The F-219/F-223 shoulde be both fixed now.

Only minor thing: would it be possible to add more test cases for the str[n[casecmp, including non-alpha characters, since we removed the isalpha condition from there?

e.g. strncasecmp("ab1c", "ab2c", 4), strcasecmp("1abc", "1abc") etc.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug (finding 219) in custom string compare functions (strcmp, strcasecmp, strncasecmp) that could produce false equality when one string is a prefix of another. The fix replaces the old loop logic with a correct approach that properly handles null terminators and prefix cases.

Changes:

  • Fix strcmp, strcasecmp, and strncasecmp in src/string.c to correctly handle prefix strings and avoid false equality
  • Add regression test cases in unit-string.c and update the test infrastructure (Makefile, .gitignore, test.mk)
  • Refactor wolfBoot_find_header in src/libwolfboot.c to use integer arithmetic instead of pointer arithmetic to avoid potential UB

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/string.c Core bug fix: rewrite compare functions to correctly handle prefix/terminator cases
tools/unit-tests/unit-string.c Regression tests for the fixed string compare functions
tools/unit-tests/Makefile Adds ASAN mode support and -fno-builtin for unit-string target
tools/test.mk Updates binary size limits to reflect the code changes
src/libwolfboot.c Refactors wolfBoot_find_header to use uintptr_t arithmetic, fixing potential pointer UB
lib/wolfssl Bumps wolfssl submodule to a newer commit
.gitignore Adds new unit test binaries to ignore list

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@padelsbach padelsbach force-pushed the padelsbach/finding-219 branch from 6f3e2a0 to d3e8fea Compare March 6, 2026 03:57
@padelsbach
Copy link
Contributor Author

This looks like a safety improvement in general. The F-219/F-223 shoulde be both fixed now.

Only minor thing: would it be possible to add more test cases for the str[n[casecmp, including non-alpha characters, since we removed the isalpha condition from there?

e.g. strncasecmp("ab1c", "ab2c", 4), strcasecmp("1abc", "1abc") etc.

Good point. I added a few test cases.

I initially went a bit overboard trying to design a much more stressful set of tests, but decided against it in favor of readability and simplicity. Let me know if you want something more sophisticated.

@padelsbach padelsbach force-pushed the padelsbach/finding-219 branch from d3e8fea to 5a7f133 Compare March 6, 2026 05:04
@danielinux danielinux self-requested a review March 6, 2026 18:24
@danielinux danielinux merged commit 260a78e into wolfSSL:master Mar 6, 2026
314 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants