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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.

This is a very neat checker, the source code reads so easily, we might as well put it as the official CERT rule description.

I think adding the non-compliant and compliant code examples would be nice. I also wrote some inline comments, but I'm fine with leaving them for a later patch. LGTM, granted that @NoQ and @Charusso are happy.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h:16-22
+extern const char *const CoreFoundationObjectiveC;
+extern const char *const LogicError;
+extern const char *const MemoryRefCount;
+extern const char *const MemoryError;
+extern const char *const UnixAPI;
+extern const char *const CXXObjectLifecycle;
+extern const char *const SecurityError;
----------------
We could turns these into `llvm::StringLiteral`s one day.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/AllocationState.h:28-30
+/// \returns The MallocBugVisitor.
+std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym);
+
----------------
Is this deadcode now?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:3383-3386
+std::unique_ptr<BugReporterVisitor> getMallocBRVisitor(SymbolRef Sym) {
+  return std::make_unique<MallocBugVisitor>(Sym);
+}
+
----------------
And this?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:50
+  StringRef ErrorMsg =
+      "'putenv' function should not be called with auto variables";
+  ExplodedNode *N = C.generateErrorNode();
----------------
How about `The 'putenv' function should not be called with arguments that have automatic storage`. But I guess we could leave wordsmithing for later.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/PutenvWithAutoChecker.cpp:55
+  // Track the argument.
+  if (isa<StackSpaceRegion>(MSR)) {
+    bugreporter::trackExpressionValue(Report->getErrorNode(), ArgExpr, *Report);
----------------
This should always be true, since we bail out a couple lines up if it isn't, right?


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

https://reviews.llvm.org/D71433