[PATCH] D42645: New simple Checker for mmap calls

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hello David,

Do you have any results of this checker on the real code? If yes, could you please share them?
There are also some inline comments regarding implementation.



================
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:42
+
+void MmapWriteExecChecker::checkPostStmt(const CallExpr *CE,
+                                         CheckerContext &C) const {
----------------
For analysis of function arguments, PreCall callback is more suitable because it is called earlier.


================
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:49
+  if (FName == "mmap") {
+    SVal ProtVal = C.getSVal(CE->getArg(2)); 
+    Optional<nonloc::ConcreteInt> Prot = ProtVal.getAs<nonloc::ConcreteInt>();
----------------
We need to check that the function call has at least three arguments to avoid crashing on weird redeclarations like `void mmap()`.


================
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:51
+    Optional<nonloc::ConcreteInt> Prot = ProtVal.getAs<nonloc::ConcreteInt>();
+    int64_t prot = Prot->getValue().getSExtValue();
+
----------------
Please follow LLVM naming conventions: variable names should start with capital.


================
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:54
+    if ((prot & (ProtWrite | ProtExec)) == (ProtWrite | ProtExec)) {
+      if (!BT) {
+        BT.reset(new BugType(this, "W^X check fail", "Write Exec prot flags set"));
----------------
Looks like this check is written in the way that allows to emit warning only once. Did you mean:
```
if (!BT)
   BT.reset(new BugType(this, "W^X check fail", "Write Exec prot flags );
ExplodedNode *N = C.generateErrorNode();
if (!N)
  return;
...
```
?


================
Comment at: lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp:55
+      if (!BT) {
+        BT.reset(new BugType(this, "W^X check fail", "Write Exec prot flags set"));
+        ExplodedNode *N = C.generateErrorNode();
----------------
Could you please give more informative warning message describing the situation and why is it harmful? Note that if user has more information, he can fix the problem faster.


Repository:
  rC Clang

https://reviews.llvm.org/D42645