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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 5 inline comments as done.
Charusso added a comment.

Thanks for the feedback! Given that it will remain an `alpha` checker for a long time (~1 year), no one really should use it.



================
Comment at: clang/docs/analyzer/checkers.rst:1935
+
+alpha.security.cert.str.31c
+"""""""""""""""""""""""""""
----------------
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.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h:22
+extern const char *const CXXObjectLifecycle;
+extern const char *const SecurityError;
+} // namespace categories
----------------
balazske wrote:
> Are there already not other checkers that find security related bugs (the taint checker?)? Why do these not use a `SecurityError`? It is not bad to have a `SecurityError` but maybe there is a reason why was it not there already. If these categories are exclusive it is hard to find out what problem (probably already existing bug type in other checkers) belongs to what category (it can be for this checker `UnixAPI` or `MemoryError` too?). 
We have only three pure security checkers: `security.insecureAPI`, `security.FloatLoopCounter` (FLP30-C, FLP30-CPP) and `alpha.security.cert.pos.34c`. The non-alpha checkers are very old, but Ted definitely made that category:
```
  const char *bugType = "Floating point variable used as loop counter";
  BR.EmitBasicReport(bugType, "Security", os.str().c_str(),
                     FS->getLocStart(), ranges.data(), ranges.size());
```
- https://github.com/llvm/llvm-project/commit/9c49762776b4d2cb16911dec0c873c90a6508968

Every other security-ish checker does not catch the CERT rule's examples. May if the checker evolves I would pick MemoryError, but it will not evolve soon.


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

https://reviews.llvm.org/D70411