balazske added inline comments.
================
Comment at: clang/docs/analyzer/checkers.rst:1935
+
+alpha.security.cert.str.31c
+"""""""""""""""""""""""""""
----------------
Charusso wrote:
> Szelethus wrote:
> > balazske wrote:
> > > There are already more checkers that can check for CERT related problems
> > > but not specially made for these. These checkers do not reside in this
> > > new `cert` group. And generally a checker does not check for specifically
> > > a CERT rule, instead for more of them or other things too, or more
> > > checkers can detect a single rule. (And the user can think that only
> > > these CERT rules are checkable that exist in this package, that is not
> > > true.) So I do not like the introduction of this new `cert` package. (The
> > > documentation of existing checkers lists if the checker is designed for a
> > > CERT rule.)
> > I disagree to some extent. I think it would be great to have a `cert`
> > package that houses all checkers for each of the rules with the addition of
> > checker aliases. Clang-tidy has something similar as well!
> I designed the checker only for the rule STR31-C that is why I have picked
> package `cert`. Clang-Tidy could aliasing checks. For example the check could
> be in the `bugprone` category aliased to `cert` and both could trigger the
> analysis.
>
> > the user can think that only these CERT rules are checkable
> We only need to move `security.FloatLoopCounter` under the `cert` package.
> What else SEI CERT rules are finished off other than package `cert`? It is
> not my fault if someone could not package well or could not catch most of the
> issues of a SEI CERT rule or could not reach the SEI CERT group to note the
> fact the Analyzer catch a very tiny part of a rule. However, this patch
> package well and could catch most of the STR31-C rule.
So I can move this checker: D71510 into `alpha.security.cert.err.33c`?
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:803
+
+} // end "cert.str"
+
----------------
Charusso wrote:
> 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?
First sentence is OK, the later part relatively better. Probably the
explanations about why it is good to turn it on or off can be left out, these
are misunderstandable. (If the manual null-termination is done, in one case it
is a "common idiom but can be misused so make warning" on other case "these
warnings generates only extra noise".)
It can be something like this (if correct):
```
Whether the checker wars on the function calls immediately or warns on bugprone
hand-written null-termination of function call made strings. Default value is
on because null-terminating the string by hand after the insecure function call
could be misused. But it is a common idiom to manually null-terminate the
strings immediately after the function calls so it could be turned off to
reduce noise of the checker.
```
I do not like the word "bugprone", is it correct english? ("Problematic" or
"insecure" could be used instead.)
================
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}}
----------------
Charusso wrote:
> 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.
There must be some misunderstanding here, I still do not know what this test
does. Can probably somebody other explain it? After the first `strcpy` the
`src` content is copied into `dest`. If `src` is not a null-terminated string
this is itself a bug and `strcpy` is undefined behavior. If `src` has
`strlen(src) > 127` it is a bug again because the copy can write outside of
`dest` past its end, into the stack frame of the function. After this happens,
writing a null terminator does not correct the other overwritten data that
still can cause crash or other thing. (This `strcpy` call is the place for a
warning. Probably not a fatal error because it is not sure, if size of `src` is
not known.) Otherwise result of `strcpy` is a null-terminated string in `dest`.
The next `*dest = '\0'` causes that `dest` points to an empty string, why is
this needed here (it was already null terminated)? The following `strcpy`
overwrites it anyway.
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
