[PATCH] D71033: [analyzer] CERT STR rule checkers: STR32-C

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware requested changes to this revision.
baloghadamsoftware added inline comments.
This revision now requires changes to proceed.
Herald added a subscriber: rnkovacs.


================
Comment at: clang/docs/analyzer/checkers.rst:1982
+It warns on reading non-null-terminated strings. This warning is restricted to
+the allocations which the Static Analyzer models with :ref:`unix.Malloc<unix-Malloc>`_.
+
----------------
Why? Is retrieving the size of arrays is so much more difficult? We miss the first example of the rule this way.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:181
       << '\'';
+  std::string ReportMessage = Out.str().str();
 
----------------
Why do we need this? The constructor of `PathSensitiveBugReport` takes a `StringRef`. `Msg.str()` returns a `StringRef`. Your solution creates a C++ string first, which means an unnecessary copy.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:313
+
+  while (Ty->isPointerType())
+    Ty = Ty->getPointeeType();
----------------
Do we really need to handle indirect pointers here? I means e.g. `char ***`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:322
+
+/// Check whether the memory allocation could overflow, if so emit a report.
+static bool isAllocationOverflow(SVal L, SVal V, const Stmt *S,
----------------
What does it mean that //the memory allocation could overflow//?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:457
 
 void StrCheckerBase::checkPostCall(const CallEvent &Call,
                                    CheckerContext &C) const {
----------------
Why checking `PostCall` and not `PreCall`? Usually `PostCall` is used for modeling and `PreCall` for checking because for modeling we need the call already evaluated (e.g. we need the return value) but checking should happen //before// starting the evaluation of the call.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:469
+    return;
+
+  // If the passed parameter is a const-qualified string we assume that the
----------------
Do we chack //any// call here passing non-null terminated strings? I disagree with that appraocah because there may be functions expecting e.g. binary data. It seems that we are not even checking the type so we could get false positives here for any `mem...()` function.


================
Comment at: clang/test/Analysis/cert/str32-c.cpp:10
+
+void *realloc(void *memblock, size_t size);
+
----------------
Please move this to the //system header simulator// instead of repeating it in every test file.


================
Comment at: clang/test/Analysis/cert/str32-c.cpp:13
+namespace test_strncpy_bad {
+enum { STR_SIZE = 32 };
+
----------------
Why `enum`?


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

https://reviews.llvm.org/D71033