Charusso marked 2 inline comments as done.
Charusso added a comment.
The false positive suppression by going backwards on the bug-path of stored
reports was a very simple concept, which turned out to be a useless one. The
rough idea was that to invalidate every report in a path, and every other
report would be left because they are reachable from a different path and they
are valid errors. So that:
void test_wonky_null_termination(const char *src) {
char dest[128];
strcpy(dest, src);
dest[sizeof(dest) - 1] = '\0';
do_something(dest); // no-warning
}
The above example null-terminates the string on every path, so we remove the
report, and:
void test_may_not_every_path_null_terminated(const char *src) {
char dest[128];
strcpy(dest, src);
if (strlen(src) > sizeof(dest)) {
dest[sizeof(dest) - 1] = '\0';
}
do_something(dest);
// expected-warning@-1 {{'dest' is not null-terminated}}
}
We say that the above example has a path where we cannot invalidate the report.
Invalidating one single control-flow path I think should be safe and keeps
every other report like the above examples. In general there is no such simple
case exist because we check for the destination array being null so we
encounter a state-split whatever we do. Also may it was a bad idea to rewrite
the `BugReporter` for a simple checker.
This null-termination-by-hand made false positives because even the buffer
could overflow, we stop up the flow, so the Vasa will not sink. I wanted to
emit reports on problematic function calls, but changing the main logic to emit
an error on the string-reading made the null-termination store-able. So that we
could drop this backward path-traversing logic, that is why I have not answered
yet. However, if we still want to emit an error on function-calls, and every of
them, we need to be able to chain the calls to the same region, which made by
the reports in the previous revisions. Also may we would use this
report-storing idea later.
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:801
+ Dependencies<[StrCheckerBase]>,
+ Documentation<HasDocumentation>;
+
----------------
In my mind the documentation is the CERT rule's page. Is it okay?
================
Comment at: clang/test/Analysis/cert/str31-c-fp-suppression.cpp:15
+
+void do_something(char *destfer);
+
----------------
Facepalm.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D70411/new/
https://reviews.llvm.org/D70411
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
