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