steakhal wrote:
I've found two more wrong models:
```diff
diff --git a/clang/test/Analysis/string-search-modeling.c
b/clang/test/Analysis/string-search-modeling.c
index 056ee37f427f..dcc2cef299a5 100644
--- a/clang/test/Analysis/string-search-modeling.c
+++ b/clang/test/Analysis/string-search-modeling.c
@@ -444,6 +444,29 @@ void test_strpbrk_before_null(void) {
clang_analyzer_eval(strpbrk(s, "a") == s); // expected-warning {{TRUE}}
}
+// --- Embedded null in the SEARCH argument (needle / accept set) ---
+// C truncates the second argument at its first null, just like the haystack.
+// The modeling does not yet do this for the needle/accept set, so these cases
+// are currently mishandled. See getCStr() applied only to the haystack in
+// CStringChecker::evalStrstr/evalStrpbrk.
+void test_strstr_needle_embedded_null(void) {
+ // C strstr searches only "cd" (needle truncated at the null), which is
+ // present in "abcd", so the real result is non-null.
+ // FIXME: should be FALSE; the needle is not truncated at its embedded null,
+ // so the model searches for the 4-byte sequence "cd\0e", fails to find it,
+ // and wrongly eliminates the found branch.
+ clang_analyzer_eval(strstr("abcd", "cd\0e") == 0); // expected-warning
{{TRUE}}
+}
+
+void test_strpbrk_accept_embedded_null(void) {
+ // C strpbrk's accept set is just "a" (truncated at the null); "xb" contains
+ // no 'a', so the real result is null.
+ // FIXME: should be TRUE; the accept set is not truncated at its embedded
+ // null, so the model treats 'b' as acceptable, matches it, and wrongly
+ // returns a non-null pointer.
+ clang_analyzer_eval(strpbrk("xb", "a\0b") == 0); // expected-warning
{{FALSE}}
+}
+
// --- memchr with pointer past embedded null ---
void test_memchr_pointer_past_null(void) {
const char *s = "1ab\0cdf\0qwrt";
```
---
I've just noticed that you have force-pushed a couple of times, and that
prompted me of explaining my preferrence of doing reviews on GitHub, for the
analyzer. This wasn't really a bit deal here on this PR, but might come handing
in the future if you stick around:
This is sort of unwritten in the llvm dev policy, but because we are using
GitHub, and [GitHub behaves
poorly](https://matthew-b-groves.medium.com/matts-tidbits-23-why-to-avoid-force-pushing-691d3abbab48)
for force-pushes, generally one should prefer not force-pushing their branch
once you are under review. Just prefer merging in from main if you need to, and
don't worry about interim commits on your branch - those will be squashed
anyway.
Another benefit of preserving interim commits is that with those you can target
individual suggestions and post a commit hash as a reply to a comment,
demonstraing the fix with that commit - this really helps reviewers because
they have a gradual exposure to the fixes you applied making it really quick to
verify and then resolve a comment.
https://github.com/llvm/llvm-project/pull/210154
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
