[PATCH] D14014: Checker of proper vfork usage

[Yury Gribov via cfe-commits]

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