[PATCH] D71433: [analyzer] CERT: POS34-C

[Csaba Dabis via Phabricator via cfe-commits]

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