[PATCH] D14014: Checker of proper vfork usage

[Vedant Kumar via cfe-commits]

vsk added a subscriber: vsk.
vsk added a comment.

Thanks for the patch.

I didn't know vfork() is in regular use. Afaict copy-on-write fork() and threading both solve a similar (the same?) problem. I'm also not convinced that flagging all stores in the vfork child process is the right thing to do, since the FP rate could be quite high. For example, doing 'x = malloc(4); *x = 0;' in the child process seems fine to me.

Some inline comments --


================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:10
@@ +9,3 @@
+//
+//  This file defines chroot checker, which checks improper use of chroot.
+//
----------------
This comment needs an update.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:38
@@ +37,3 @@
+//                                  |
+//                                  --return--> bug
+class VforkChecker : public Checker<check::PreCall, check::PostCall,
----------------
Please clarify this comment -- it might be better to state a list of specific buggy conditions.

E.g, are writes into heap-allocated allowed? Where are return statements allowed?

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:44
@@ +43,3 @@
+
+  // This bug refers to dangerous code in vforked child.
+  mutable std::unique_ptr<BuiltinBug> BT_BadChildCode;
----------------
This comment isn't needed -- a description to this effect can go in the top-of-file comment.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:65
@@ +64,3 @@
+
+  // these constructs are prohibited in vforked child
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
----------------
This comment doesn't add much, please remove it.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:75
@@ +74,3 @@
+REGISTER_TRAIT_WITH_PROGRAMSTATE(VforkLhs, const void *)
+#define VFORK_LHS_NONE ((void *)(uintptr_t)1)
+
----------------
Why not nullptr? If there's a good reason, please explain it with a comment.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:147
@@ +146,3 @@
+    return D;
+  } while (0);
+
----------------
This is hard to read, please simplify this. E.g;

  if (PD = dyn-cast(P)) { /* handle cases where P is-a DeclStmt */ }
  else if (Assign = dyn-cast(P)) { /* handle cases where P is-a binop */ }

Using loop constructs here shouldn't be necessary.

================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:167
@@ +166,3 @@
+  // otherwise no lhs
+  return (const VarDecl *)VFORK_LHS_NONE;
+}
----------------
Typically nullptr is used here.

================
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();
----------------
Actually, if this is the case, wouldn't it be worthwhile to flag calls to vfork() in child processes?


Repository:
  rL LLVM

http://reviews.llvm.org/D14014