[PATCH] D14014: Checker of proper vfork usage

[Yury Gribov via cfe-commits]

ygribov added a comment.

> One thing to note (which I assume you are already aware of) is that we already have the "security.insecureAPI.vfork" checker,

>  an AST check that warns on *every* use of vfork.


Yes, I was aware of this one. Unfortunately as I mentioned above vfork is probably to stay with us for quite a while.

> This check is on by default (which I think makes sense, given the variety of ways that vfork is problematic) --

>  so users of your more permissive check would have to disable the default check.


This is a valid concern because it greatly complicates enablement of VforkChecker for a casual user. Do we have some mechanism so that one checker (here VforkChecker) could disable/modify behavior of it's less precise companion (insecureAPI)? This sounds like a generally useful feature (although it damages the modularity of checkers).


================
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:360
@@ +359,3 @@
+def VforkChecker : Checker<"Vfork">,
+  HelpText<"Check for proper usage of vfork.">,
+  DescFile<"VforkChecker.cpp">;
----------------
dcoughlin wrote:
> The convention here is to not have a '.' after the help text.
Got it.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:61
@@ +60,3 @@
+
+  // these are used to track vfork state
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
----------------
dcoughlin wrote:
> LLVM convention is to use capitalization and punctuation for comments. (See http://llvm.org/docs/CodingStandards.html#commenting).
Thanks, I should probably re-read the std once again.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:74
@@ +73,3 @@
+// region child is allowed to write)
+REGISTER_TRAIT_WITH_PROGRAMSTATE(VforkLhs, const void *)
+#define VFORK_LHS_NONE ((void *)(uintptr_t)1)
----------------
dcoughlin wrote:
> Is it possible to use a name that implies a more semantic notion? For example, "VforkResultRegion"?
That would be the fourth iteration of the name) But yes, "Result region" sounds more descriptive.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:102
@@ +101,3 @@
+
+  for (SmallVectorImpl<const IdentifierInfo *>::iterator
+         I = VforkWhitelist.begin(), E = VforkWhitelist.end();
----------------
dcoughlin wrote:
> I think it is better to use SmallSet for VforkWhitelist rather than duplicate that functionality here.
Ok.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:115
@@ +114,3 @@
+    BT_BadChildCode.reset(
+      new BuiltinBug(this, "Dangerous construct in vforked process",
+                     "Prohibited construct after successful "
----------------
dcoughlin wrote:
> What do you think about making the warnings more descriptive here? For example, you could one warning saying that "Child can only modify variable used to store result of vfork()", one that says "Child can only call _exit() or `exec()` family of functions", "Child must not return from function calling vfork()", etc.
> 
> These descriptions would help the user not have to guess what they are doing wrong.
Yes, given the public ignorance about vfork's idiosyncrasies that makes good sense.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:118
@@ +117,3 @@
+                     "vfork"));
+  ExplodedNode *N = C.generateSink(C.getState(), C.getPredecessor());
+  auto Report = llvm::make_unique<BugReport>(*BT_BadChildCode, 
----------------
dcoughlin wrote:
> You should use `C.generateErrorNode()` here, which expresses the intent to create a node for the purposes of error reporting.
Will do.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:135
@@ +134,3 @@
+  do {
+    const DeclStmt *PD = dyn_cast_or_null<DeclStmt>(P);
+    if (!PD)
----------------
dcoughlin wrote:
> For dyn_cast and friends and you use `auto` to avoid repeating the type twice. For example:
> 
> ```
> const auto *PD = dyn_cast_or_null<DeclStmt>(P);
> ```
True. This was forward-ported from our ancient version so lacks C++11 beauties.

================
Comment at: test/Analysis/vfork-1.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core.builtin.NoReturnFunctions,alpha.unix.Vfork %s -verify
+
----------------
dcoughlin wrote:
> Tests generally need to have all of core turned on (e.g., `-analyzer-checker=core,alpha.unix.Vfork`) because the analyzer implements key functionality in the core checkers. (It is not safe to run a path-sensitive checker without core turned on).
Ok, good to know.

================
Comment at: test/Analysis/vfork-1.c:68
@@ +67,3 @@
+  if (vfork() == 0)
+    _exit(1);
+  x = 0;
----------------
dcoughlin wrote:
> I think it would be good to add `// no-warning` on the lines that shouldn't warn. This will make it easier for people tracking down test failures to know that there really shouldn't be a warning on that line.
Sounds like a good maintenance practice, I'll definitely add it.

================
Comment at: test/Analysis/vfork-2.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.unix.Vfork %s -verify
+
----------------
dcoughlin wrote:
> Is it possible to combine this with the other test file? If not, can you rename the files to be more descriptive than "-1" or "-2". This will help people adding future tests decide which file they should go in.
Vfork-2.c is actually dedicated to model the bug in libxtables. I can surely merge both tests.


Repository:
  rL LLVM

http://reviews.llvm.org/D14014