[PATCH] D14014: Checker of proper vfork usage

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Thanks for the patch!

Some minor comments inline.

What happens when this checker and the security.insecureAPI.vfork are enabled at the same time?

Did you run this checker on a large body of code? Did it find any issues? What is needed to turn this into a non-alpha checker?


================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:10
@@ +9,3 @@
+//
+//  This file defines vfork checker which checks for dangerous uses of vfork.
+//
----------------
Let's move the more detailed comment that describes what the checker does here.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:44
@@ +43,3 @@
+// Pattern matches to extract lhs in `lhs = vfork()' statement.
+static const VarDecl *GetAssignedVariable(const CallEvent &Call,
+                                   CheckerContext &C) {
----------------
This should be added as a utility function. Does this exist elsewhere?

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:63
@@ +62,3 @@
+      return D;
+  } while (0);
+
----------------
Can this be rewritten so that it is more clear why this terminates? 
Also, I'd prefer to use "while(true)".

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:155
@@ +154,3 @@
+    if (!BT)
+      BT.reset(new BuiltinBug(this, "Dangerous construct in vforked process"));
+
----------------
"a vforked process"?

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:160
@@ +159,3 @@
+
+    os << What << " is prohibited after successful vfork";
+
----------------
"after successful vfork" -> "after a successful vfork"?
Devin, are we missing an article here and in the other error messages?

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:163
@@ +162,3 @@
+    auto Report = llvm::make_unique<BugReport>(*BT, os.str(), N);
+    C.emitReport(std::move(Report));
+  }
----------------
Ideally, we would point out where the vfork has occurred along the path with a BugReportVisitor. (But it's not a blocker.)

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:208
@@ +207,3 @@
+      && !isCallWhitelisted(Call.getCalleeIdentifier(), C))
+    reportBug("Calling functions (except exec(3) or _exit(2))", C);
+}
----------------
We are not listing the full whitelist here. Should we drop the "(except ..)" from the message?

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:226
@@ +225,3 @@
+
+  reportBug("Assigning variables (except return value of vfork)", C);
+}
----------------
"except return value" -> "except the return value"? Or maybe we should drop the "(except ...)" here as well to make the message shorter.

================
Comment at: test/Analysis/vfork.c:24
@@ +23,3 @@
+    // Ensure that writing variables is prohibited.
+    x = 0; // expected-warning{{}}
+    break;
----------------
We should check for the exact expected warning in regression tests.


http://reviews.llvm.org/D14014