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