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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
