[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Looks great. I only have nits mostly for the docs.



================
Comment at: clang/docs/analyzer/checkers.rst:2334-2336
+modified without causing undefined behavior. These functions
+include getenv(), setlocale(), localeconv(), asctime(), and
+strerror(). In such cases, the function call results must be
----------------
IMO the list of functions should be mentioned separately. What if the list gets longer and longer.
A separate paragraph could list these instead. Also, wrap them with double backticks.
```
``getenv()``
```


================
Comment at: clang/docs/analyzer/checkers.rst:2340
+Checker models return values of these functions as const
+qualified. Their modification is checked in StoreToImmutable
+core checker.
----------------
Use crossref links.


================
Comment at: clang/docs/analyzer/checkers.rst:2343-2351
+.. code-block:: c
+
+  void writing_to_envvar() {
+    char *p = getenv("VAR"); // note: getenv return value
+                             // should be treated as const
+    if (!p)
+      return;
----------------
I don't like the fact that this example works only if the `alpha.cert.pos.StoreToImmutableChecker` is enabled, which is an //alpha// checker.

There are other places where such immutable memory regions arise, such as writable strings.
Anyway, at least let's mention that the given alpha checker has anything to do with this checker (+ crossref!).


================
Comment at: clang/docs/analyzer/checkers.rst:2352
+  }
+
 alpha.security.taint
----------------
You should also embed an example of how to fix this issue: E.g. advertises making a copy of the immutable region to a mutable location and doing the mutation there instead. And use `const` as a preventive measure for the future for the original pointer.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:767-769
     assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg) ||
-           isa<GlobalSystemSpaceRegion>(sreg));
+           isa<GlobalSystemSpaceRegion>(sreg) ||
+           isa<GlobalImmutableSpaceRegion>(sreg));
----------------
Please merge these into a single `isa<T1, T2,...>()`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:68-70
   MIGChecker.cpp
+  cert/ModelConstQualifiedReturnChecker.cpp
   MoveChecker.cpp
----------------
Put this in the right alphabetical place.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:27-29
+  BugType ImmutableMemoryBind{this,
+                              "Immutable memory should not be overwritten",
+                              categories::MemoryError};
----------------
You could expose a pointer to this by creating a `const BugType *getBugType() const;` getter method, which could be used by other checkers.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:51
+  auto Report = std::make_unique<PathSensitiveBugReport>(
+      ImmutableMemoryBind, "modifying immutable memory", ErrorNode);
+  Report->markInteresting(R);
----------------
We capitalize the static analyzer warnings and notes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp:1-2
+//== ModelConstQualifiedReturnChecker.cpp ----------------------- -*- C++
+//-*--=//
+//
----------------
Fix the comment.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp:10-11
+//
+// This file defines ModelConstQualifiedReturnChecker
+// Return values of certain functions should be treated as const-qualified
+//
----------------
Comments should be capitalized and punctuated as usual.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp:30-43
+class ModelConstQualifiedReturnChecker : public Checker<eval::Call> {
+private:
+  void evalConstQualifiedReturnCall(const CallEvent &Call,
+                                    CheckerContext &C) const;
+
+  // SEI CERT ENV30-C
+  const CallDescriptionSet ConstQualifiedReturnFunctions = {
----------------
I can see a small issue by `evaCall`-ing these functions.
The problem is that we might not model all aspects of these functions, thus the modeling can cause harm to the analysis. E.g. by not doing invalidation where we actually would need invalidation to kick in, or anything else really.
Consequently, another problem is that no other checker can evaluate these, since we evaluate them here.
Thus, fixing such improper modeling could end up in further changes down the line.
Unfortunately, I don't have any better ATM, so let's go with this `evalCall` approach.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp:66
+      [SymReg, FunName](PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
+        if (BR.getBugType().getCheckerName() != "core.StoreToImmutable" ||
+            !BR.isInteresting(SymReg))
----------------
One should achieve this by comparing the BugType pointers.


================
Comment at: clang/test/Analysis/cert/env30-c.cpp:2
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.StoreToImmutable,alpha.security.cert.env.ModelConstQualifiedReturn \
+// RUN:  -analyzer-output=text -verify -Wno-unused %s
----------------
Also enable the 'core' package.


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

https://reviews.llvm.org/D124244