[PATCH] D70411: [analyzer] CERT: STR31-C

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+
----------------
NoQ wrote:
> `alpha.cert.str`.
This text may be hard to understand. The option means "if it is off, the problem report happens only if there is no hand-written null termination of the string"? I looked at the CERT rule but did not find out why is null termination by hand important here. (I did not look at the code to find out what it does.) And "WarnOnCall" suggests that there is warning made on calls if on, or not at all or at other location if off, is this correct?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:28
 
+/// \returns The MallocBugVisitor.
+std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym);
----------------
This documentation could be improved or left out.


================
Comment at: clang/test/Analysis/cert/str31-c-notes-warn-on-call-off.cpp:68
+
+  do_something(dest);
+  // expected-warning at -1 {{'dest' is not null-terminated}}
----------------
Maybe I have wrong expectations about what this checker does but did understand it correctly yet. The main problem here is that `dest` may be not large enough (if size of `src` is not known). This would be a better text for the error message? And if this happens (the write outside) it is already a fatal error, can cause crash. If no buffer overflow happens the terminating zero is copied from `src`, so why would `dst` be not null terminated?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70411/new/

https://reviews.llvm.org/D70411