ygribov added a comment.
> I didn't know vfork() is in regular use.
Neither did I, until I had to debug the damned code. Actually Android has some
5 or 6 uses of it.
> 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.
I also have these concerns. Manpage is very strict about behavior of vforked
processes (for a good reason though). Currently most uses of vfork are
incorrect according to this checker.
I think there is some potential to relax the constraints:
- at the very least we can allow calls to functions marked as inline as those
won't result in any memory writes
- we can even allow to call any functions which are guaranteed to not change
global state
> For example, doing 'x = malloc(4); *x = 0;' in the child process seems fine
> to me.
I also thought about similar examples. Here are some not so obvious
side-effects:
- you'll get a memory leak (as malloc will be shared by parent and child
processes)
- in the following case behavior of parent is undefined because original value
of variable will be corrupted:
x = some_very_important_lost;
if (vfork()==0) {
x = ...; // some_very_important_data is lost
_exit(1);
}
// parent code
if (some_very_important_data) {
// UB
}
- even if child does not explicitly write any parent's variables, compiler may
decide to merge stack slots so write into a local variable in child process may
corrupt a seemingly unrelated variable in parent
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:10
@@ +9,3 @@
+//
+// This file defines chroot checker, which checks improper use of chroot.
+//
----------------
vsk wrote:
> This comment needs an update.
Yikes, thanks for pointing this out.
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:38
@@ +37,3 @@
+// |
+// --return--> bug
+class VforkChecker : public Checker<check::PreCall, check::PostCall,
----------------
vsk wrote:
> 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?
This primitive scheme simply mirrors the manpage which is very strict about
this - write to _any_ data besides return value of vfork is disallowed, ditto
for function calls (and so "return" only applies to current function).
================
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;
----------------
vsk wrote:
> This comment isn't needed -- a description to this effect can go in the
> top-of-file comment.
Right.
================
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;
----------------
vsk wrote:
> This comment doesn't add much, please remove it.
Ok.
================
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)
+
----------------
vsk wrote:
> Why not nullptr? If there's a good reason, please explain it with a comment.
Ok, this indeed deserves a comment. FTR: nullptr means parent process,
VFORK_LHS_NONE means vforked process without explicit LHS and rest means LHS
variable.
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:147
@@ +146,3 @@
+ return D;
+ } while (0);
+
----------------
vsk wrote:
> 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.
In this case each statement depends on the previous one so we'll have to resort
to deeply nested conditionals. I thought that do-while-false is a common
pattern for simplifying such constructs but I may be wrong.
================
Comment at: lib/StaticAnalyzer/Checkers/VforkChecker.cpp:167
@@ +166,3 @@
+ // otherwise no lhs
+ return (const VarDecl *)VFORK_LHS_NONE;
+}
----------------
vsk wrote:
> Typically nullptr is used here.
See my comment above, these two encode two different things.
================
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();
----------------
dcoughlin wrote:
> 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.
They will be flagged in checkPreCall. But I should probably add a comment as
you're the second reviewer who complained about this.
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
