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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks! This looks like a simple and efficient check. I have one overall suggestion.

Currently the check may warn on non-bugs of the following kind:

  void foo() {
    char env[] = "NAME=value";
    putenv(env);
    doStuff();
    putenv("NAME=anothervalue");
  }

I.e., it's obviously harmless if the local variable pointer is removed from the environment before it goes out of scope. Can we instead warn when the *last* `putenv()` on the execution path through the current stack frame is a local variable (that goes out of scope at the end of the stack frame)?

That'd allow the checker to be enabled by default, which will give a lot more users access to it. Otherwise we'll have to treat it as an opt-in check and users will only enable it when they know about it, which is much less usefulness.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:27
+class PutenvWithAutoChecker : public Checker<check::PostCall> {
+  mutable std::unique_ptr<BugType> BT;
+
----------------
The modern idiom is `BugType BT{this, "Bug kind", "Bug category"};` - and drop the lazy initialization as well.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:35-37
+  if (const IdentifierInfo *II = Call.getCalleeIdentifier())
+    if (!II->isStr("putenv"))
+      return;
----------------
The modern idiom here is `CallDescription`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:42-43
+  const Expr *ArgExpr = Call.getArgExpr(0);
+  const MemSpaceRegion *MSR =
+      ArgV.getAsRegion()->getBaseRegion()->getMemorySpace();
+
----------------
You can call `getMemorySpace()` directly on any region.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:45-52
+  if (const auto *DRE = dyn_cast<DeclRefExpr>(ArgExpr->IgnoreImpCasts()))
+    if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()))
+      IsPossiblyAutoVar = isa<ParmVarDecl>(VD) && isa<UnknownSpaceRegion>(MSR);
+
+  if (!IsPossiblyAutoVar &&
+      (isa<HeapSpaceRegion>(MSR) || isa<StaticGlobalSpaceRegion>(MSR) ||
+       isa<GlobalSystemSpaceRegion>(MSR) ||
----------------
Simply check whether the memory space is a stack memory space. This should be a one-liner.


================
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();
----------------
Charusso wrote:
> @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.
Replied above^^


================
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));
----------------
Charusso wrote:
> 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!
I'm not sure it's valid to use `C.getPredecessor()` for tracking; it might get you into trouble for the same reason that using `C.getPredecessor()` as error node will get you into trouble. Please use the error node itself instead.


Repository:
  rC Clang

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

https://reviews.llvm.org/D71433