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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 4 inline comments as done.
Charusso added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+
----------------
balazske wrote:
> 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?
You let the buffer overflow by a bugprone function call, then you adjust the null-terminator by simply `buf[size] = '\0'`, then you made sure the buffer cannot overflow, since you have terminated it. It is a very common idiom therefore I do not think a hand-written null-termination is a security issue. The SEI CERT rules are all theoretical so you will not find anything useful in practice. My solution is practical.

> This text may be hard to understand.
Please note that this text only made for Static Analyzer developers. Let us rephrase it then:

> Whether the checker needs to warn on the bugprone function calls immediately or look for bugprone hand-written null-termination of bugprone function call made strings. It is a common idiom to null-terminate the string by hand after the insecure function call produce the string which could be misused so that it is on by default. It is useful to turn it off to reduce the noise of the checker, because people usually null-terminate the string by hand immediately after the bugprone function call produce the string.

Do you buy that?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:28
 
+/// \returns The MallocBugVisitor.
+std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym);
----------------
balazske wrote:
> This documentation could be improved or left out.
Totally, I have already forgotten I wanted to remove it. Thanks!


================
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}}
----------------
balazske wrote:
> 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?
I like that error message because that is the truth, that is the issue. If it would crash so the operating system would detect a fatal error we should not develop Static Analyzer and vulnerabilities would not exist. That would be cool, btw.


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

https://reviews.llvm.org/D70411