[PATCH] D14014: Checker of proper vfork usage

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Hi Yury,

Thanks for the patch. This is definitely interesting for upstream!

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. 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.


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

================
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;
----------------
LLVM convention is to use capitalization and punctuation for comments. (See http://llvm.org/docs/CodingStandards.html#commenting).

================
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)
----------------
Is it possible to use a name that implies a more semantic notion? For example, "VforkResultRegion"?

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

================
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 "
----------------
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.

================
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, 
----------------
You should use `C.generateErrorNode()` here, which expresses the intent to create a node for the purposes of error reporting.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:135
@@ +134,3 @@
+  do {
+    const DeclStmt *PD = dyn_cast_or_null<DeclStmt>(P);
+    if (!PD)
----------------
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);
```

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:186
@@ +185,3 @@
+                                 CheckerContext &C) const {
+  // we can't call vfork in child so don't bother
+  ProgramStateRef State = C.getState();
----------------
vsk wrote:
> Actually, if this is the case, wouldn't it be worthwhile to flag calls to vfork() in child processes?
This will get caught by `checkPreCall`, right? Because `vfork` is not in the white list.

================
Comment at: test/Analysis/vfork-1.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core.builtin.NoReturnFunctions,alpha.unix.Vfork %s -verify
+
----------------
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).

================
Comment at: test/Analysis/vfork-1.c:68
@@ +67,3 @@
+  if (vfork() == 0)
+    _exit(1);
+  x = 0;
----------------
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.

================
Comment at: test/Analysis/vfork-2.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.unix.Vfork %s -verify
+
----------------
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D14014