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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
