[PATCH] D71433: [analyzer] CERT: POS34-C
Csaba Dabis via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Dec 13 04:36:10 PST 2019
Charusso added a reviewer: aaron.ballman.
Charusso added a subscriber: aaron.ballman.
Charusso added inline comments.
================
Comment at: clang/docs/analyzer/checkers.rst:1881
+
+ #include <stdlib.h>
+
----------------
I would remove that line.
================
Comment at: clang/docs/analyzer/checkers.rst:1893
+
+This check corresponds to the CERT C Coding Standard rule.
+
----------------
I think it is clear it is a CERT-checker.
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:765
+ HelpText<"Finds calls to the `putenv` function which pass a pointer to "
+ "an automatic variable as the argument. (CERT POS 34C)">,
+ Documentation<HasDocumentation>;
----------------
I would write ##`putenv`## -> `'putenv'` and the CERT rule-number should be clear from that point so you could emit it.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:52
+ isa<GlobalSystemSpaceRegion>(MSR) ||
+ isa<GlobalImmutableSpaceRegion>(MSR) || isa<UnknownSpaceRegion>(MSR)))
+ return;
----------------
I think you could shrink that into:
```lang=c
!IsPossiblyAutoVar && !isa<StackSapceRegion>(MSR)
```
What you have specified is a negation of
```lang=c
!isa<StackSapceRegion>(MSR) && !isa<CodeSpaceRegion>(MSR) && !isa<GlobalInternalSpaceRegion>(MSR)
```
~ according to: https://clang.llvm.org/doxygen/classclang_1_1ento_1_1MemSpaceRegion.html
where the `CodeSpaceRegion` and `GlobalInternalSpaceRegion` is not that interesting for the checker.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:58
+ this, "'putenv' function should not be called with auto variables",
+ categories::SecurityError));
+ ExplodedNode *N = C.generateErrorNode();
----------------
@NoQ, it is from your documentation. Would we prefer that or the `registerChecker(CheckerManager &)` is the new way to construct the `BugType`? I believe the latter is more appropriate.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:60
+ ExplodedNode *N = C.generateErrorNode();
+ auto Report = llvm::make_unique<BugReport>(*BT, BT->getName(), N);
+ C.emitReport(std::move(Report));
----------------
We would like to point out the allocation's site, where it comes from.
We have two facilities to do so: `MallocBugVisitor` to track dynamic allocation and `trackExpressionValue()` to track any kind of an expression.
You could find examples from my CERT-checker: D70411. The rough idea is that:
```lang=c
// Track the argument.
if (isa<StackSpaceRegion>(MSR)) {
bugreporter::trackExpressionValue(C.getPredecessor(), ArgExpr, *Report);
} else if (const SymbolRef Sym = ArgV.getAsSymbol()) { // It is a `HeapSpaceRegion`
Report->addVisitor(allocation_state::getMallocBRVisitor(Sym));
}
```
~ here you need to copy-paste the `getMallocBRVisitor()` from my review. Sorry!
================
Comment at: clang/test/Analysis/cert/pos34-c.cpp:4
+// RUN: -verify %s
+
+#include "../Inputs/system-header-simulator.h"
----------------
Could you inject the rule's page please?
================
Comment at: clang/test/Analysis/cert/pos34-c.cpp:13
+
+// example from cert
+int volatile_memory1(const char *var) {
----------------
I would name the rule properly, as a sentence like: `Example from the CERT rule's page.`
My idea was that to have a `/cert/pos34-c.cpp` which only consists of the rule's page stuff mentioning the rule's page on the top. And having a separate file which only consists of arbitrary tests called like `/cert/pos34-c-fp-suppression.cpp` as it shows some false-positive suppression what you have found on tests or something. Like @aaron.ballman wrote two tests in D70823 and I would copy-paste them from his comment.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71433/new/
https://reviews.llvm.org/D71433
More information about the cfe-commits
mailing list