[PATCH] D70411: [analyzer] CERT: StrChecker: 31.c

[Csaba Dabis via Phabricator via cfe-commits]

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 at -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