[PATCH] D14014: Checker of proper vfork usage

[Yury Gribov via cfe-commits]

ygribov added a comment.

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


Both checkers will emit warnings independently (which I think is ok).

> Did you run this checker on a large body of code? Did it find any issues?


Yes, I've ran it on Android 5 and found several violations (function calls, assignments). Frankly I think that most existing uses of vfork do not obey the specification.

> What is needed to turn this into a non-alpha checker?


I guess that's question for maintainers) We have to consider that vfork is used rarely enough.


================
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) {
----------------
zaks.anna wrote:
> This should be added as a utility function. Does this exist elsewhere?
Frankly this function only handles two common patterns right now (assignment and initialization). This was enough for all uses of vfork which I've seen but I'm afraid that general case may require much more work.

I'll try to find any dups.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:63
@@ +62,3 @@
+      return D;
+  } while (0);
+
----------------
zaks.anna wrote:
> Can this be rewritten so that it is more clear why this terminates? 
> Also, I'd prefer to use "while(true)".
Actually this isn't really a loop - it's a common do-while-false idiom which allows to reduce amount of nesting. You can check for similar pattern in ArrayBoundCheckerV2.cpp and also other parts of codebase.

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

================
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));
+  }
----------------
zaks.anna wrote:
> Ideally, we would point out where the vfork has occurred along the path with a BugReportVisitor. (But it's not a blocker.)
Yeah, that would be nice. But vfork code blocks are typically very small (5-10 LOC) so it'll all be clear anyway.

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

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

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


http://reviews.llvm.org/D14014