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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
