Checker of proper vfork usage

[ygribov]

Summary

[analyzer] Add VforkChecker to find unsafe code in vforked process.

This syntactic checker looks for unsafe constructs in vforked process:
function calls (excluding whitelist), memory write and returns.
This was originally motivated by a vfork-related bug in xtables package.

Patch by Yury Gribov.

[vsk]

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

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

10 ↗

(On Diff #38218)

This comment needs an update.

38 ↗

(On Diff #38218)

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?

44 ↗

(On Diff #38218)

This comment isn't needed -- a description to this effect can go in the top-of-file comment.

65 ↗

(On Diff #38218)

This comment doesn't add much, please remove it.

75 ↗

(On Diff #38218)

Why not nullptr? If there's a good reason, please explain it with a comment.

147 ↗

(On Diff #38218)

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.

167 ↗

(On Diff #38218)

Typically nullptr is used here.

186 ↗

(On Diff #38218)

Actually, if this is the case, wouldn't it be worthwhile to flag calls to vfork() in child processes?

[dcoughlin]

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.

lib/StaticAnalyzer/Checkers/Checkers.td

360 ↗

(On Diff #38218)

The convention here is to not have a '.' after the help text.

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

61 ↗

(On Diff #38218)

LLVM convention is to use capitalization and punctuation for comments. (See http://llvm.org/docs/CodingStandards.html#commenting).

74 ↗

(On Diff #38218)

Is it possible to use a name that implies a more semantic notion? For example, "VforkResultRegion"?

102 ↗

(On Diff #38218)

I think it is better to use SmallSet for VforkWhitelist rather than duplicate that functionality here.

115 ↗

(On Diff #38218)

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.

118 ↗

(On Diff #38218)

You should use C.generateErrorNode() here, which expresses the intent to create a node for the purposes of error reporting.

135 ↗

(On Diff #38218)

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);

186 ↗

(On Diff #38218)

This will get caught by checkPreCall, right? Because vfork is not in the white list.

test/Analysis/vfork-1.c

1 ↗

(On Diff #38218)

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

68 ↗

(On Diff #38218)

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.

test/Analysis/vfork-2.c

1 ↗

(On Diff #38218)

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.

[ygribov]

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_data;
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

[ygribov]

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

10 ↗

(On Diff #38218)

Yikes, thanks for pointing this out.

38 ↗

(On Diff #38218)

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

44 ↗

(On Diff #38218)

Right.

65 ↗

(On Diff #38218)

Ok.

75 ↗

(On Diff #38218)

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.

147 ↗

(On Diff #38218)

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.

167 ↗

(On Diff #38218)

See my comment above, these two encode two different things.

186 ↗

(On Diff #38218)

They will be flagged in checkPreCall. But I should probably add a comment as you're the second reviewer who complained about this.

[dcoughlin]

For example, doing 'x = malloc(4); *x = 0;' in the child process seems fine to me.

I don't think this is necessarily safe. For example, malloc() could end up both modifying memory shared between the child and parent process but only modifying process state for the child, leaving the parent in an inconsistent state.

POSIX (before removing vfork() completely) was pretty brutal about the restrictions in the child:

...the behavior is undefined if the process created by vfork() either modifies any data other than a variable of type pid_t used to store the return value from vfork(), or returns from the function in which vfork() was called, or calls any other function before successfully calling _exit() or one of the exec family of functions.

[ygribov]

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.

Yes, I was aware of this one. Unfortunately as I mentioned above vfork is probably to stay with us for quite a while.

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.

This is a valid concern because it greatly complicates enablement of VforkChecker for a casual user. Do we have some mechanism so that one checker (here VforkChecker) could disable/modify behavior of it's less precise companion (insecureAPI)? This sounds like a generally useful feature (although it damages the modularity of checkers).

lib/StaticAnalyzer/Checkers/Checkers.td

360 ↗

(On Diff #38218)

Got it.

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

61 ↗

(On Diff #38218)

Thanks, I should probably re-read the std once again.

74 ↗

(On Diff #38218)

That would be the fourth iteration of the name) But yes, "Result region" sounds more descriptive.

102 ↗

(On Diff #38218)

Ok.

115 ↗

(On Diff #38218)

Yes, given the public ignorance about vfork's idiosyncrasies that makes good sense.

118 ↗

(On Diff #38218)

Will do.

135 ↗

(On Diff #38218)

True. This was forward-ported from our ancient version so lacks C++11 beauties.

test/Analysis/vfork-1.c

1 ↗

(On Diff #38218)

Ok, good to know.

68 ↗

(On Diff #38218)

Sounds like a good maintenance practice, I'll definitely add it.

test/Analysis/vfork-2.c

1 ↗

(On Diff #38218)

Vfork-2.c is actually dedicated to model the bug in libxtables. I can surely merge both tests.

[ygribov]

This is a valid concern because it greatly complicates enablement of VforkChecker for a casual user.

I think at the very least I can check that InsecureAPI is enable and issue a warning to user.

[a.sidorin]

I put some suggestions in inline comments.

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

47 ↗

(On Diff #38218)

I think it's a good idea to make some functions static and/or move them out of class definition.

97 ↗

(On Diff #38218)

What about combination of std::transform + std::sort + std::binary_search?

136 ↗

(On Diff #38218)

if (PD && PD->isSingleDecl()) {
  if (const VarDecl *D = dyn_cast<VarDecl>(PD->getSingleDecl())
   return D;

149 ↗

(On Diff #38218)

You can use early return to escape do{}.

196 ↗

(On Diff #38218)

Is check for non-concrete value is really required?

252 ↗

(On Diff #38218)

if (is...)
  report...

[ygribov]

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

47 ↗

(On Diff #38218)

Right. Which one is preferred btw?

97 ↗

(On Diff #38218)

I think I should use SmallSet here as suggested by Devin.

149 ↗

(On Diff #38218)

In this particular case - yes. But what's wrong about original do-while-false? I thought it's a common idiom...

196 ↗

(On Diff #38218)

This may be remains of old code, I'll re-check.

[ygribov]

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

149 ↗

(On Diff #38218)

FYI there are several uses of do-while-false in CSA codebase.

[a.sidorin]

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

47 ↗

(On Diff #38218)

isChildProcess(), getAssignedVariable() are good candidates since they don't use any class member.

[ygribov]

Updated based on review.

[ygribov]

This is a valid concern because it greatly complicates enablement of VforkChecker for a casual user.

I think at the very least I can check that InsecureAPI is enable and issue a warning to user.

Actually I think that's not a huge problem. InsecureAPI checker would issue a warning but not emit a sink node. So it wouldn't influence VforkChecker at all.

[dcoughlin]

LGTM. Can you update the summary to a commit message? Then I will commit. Thanks for upstreaming!

[ygribov]

Done!

[zaks.anna]

Thanks for the patch!

Some minor comments inline.

What happens when this checker and the security.insecureAPI.vfork are enabled at the same time?

Did you run this checker on a large body of code? Did it find any issues? What is needed to turn this into a non-alpha checker?

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

10 ↗

(On Diff #38423)

Let's move the more detailed comment that describes what the checker does here.

44 ↗

(On Diff #38423)

This should be added as a utility function. Does this exist elsewhere?

63 ↗

(On Diff #38423)

Can this be rewritten so that it is more clear why this terminates?
Also, I'd prefer to use "while(true)".

155 ↗

(On Diff #38423)

"a vforked process"?

160 ↗

(On Diff #38423)

"after successful vfork" -> "after a successful vfork"?
Devin, are we missing an article here and in the other error messages?

163 ↗

(On Diff #38423)

Ideally, we would point out where the vfork has occurred along the path with a BugReportVisitor. (But it's not a blocker.)

208 ↗

(On Diff #38423)

We are not listing the full whitelist here. Should we drop the "(except ..)" from the message?

226 ↗

(On Diff #38423)

"except return value" -> "except the return value"? Or maybe we should drop the "(except ...)" here as well to make the message shorter.

test/Analysis/vfork.c

24 ↗

(On Diff #38423)

We should check for the exact expected warning in regression tests.

[ygribov]

What happens when this checker and the security.insecureAPI.vfork are enabled at the same time?

Both checkers will emit warnings independently (which I think is ok).

Did you run this checker on a large body of code? Did it find any issues?

Yes, I've ran it on Android 5 and found several violations (function calls, assignments). Frankly I think that most existing uses of vfork do not obey the specification.

What is needed to turn this into a non-alpha checker?

I guess that's question for maintainers) We have to consider that vfork is used rarely enough.

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

44 ↗

(On Diff #38423)

Frankly this function only handles two common patterns right now (assignment and initialization). This was enough for all uses of vfork which I've seen but I'm afraid that general case may require much more work.

I'll try to find any dups.

63 ↗

(On Diff #38423)

Actually this isn't really a loop - it's a common do-while-false idiom which allows to reduce amount of nesting. You can check for similar pattern in ArrayBoundCheckerV2.cpp and also other parts of codebase.

155 ↗

(On Diff #38423)

Right.

163 ↗

(On Diff #38423)

Yeah, that would be nice. But vfork code blocks are typically very small (5-10 LOC) so it'll all be clear anyway.

208 ↗

(On Diff #38423)

What about

except exec*() or _exit()

?

226 ↗

(On Diff #38423)

I'd rather keep it.

test/Analysis/vfork.c

24 ↗

(On Diff #38423)

Ok.

[ygribov]

Updated after Anna's review.

[ygribov]

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

45 ↗

(On Diff #38740)

It seems that other checkers do more or less the same throw-away predicates (e.g. see isAssignmentOp in DereferenceChecker.cpp).

164 ↗

(On Diff #38740)

I've added a TODO.

[zaks.anna]

lib/StaticAnalyzer/Checkers/VforkChecker.cpp

45 ↗

(On Diff #38740)

Frankly this function only handles two common patterns right now

It seems that other checkers do more or less the same throw-away
predicates

Both of these just highlight that it's best to make this a utility function:

• New checkers will not have to roll their own predicates.
• The reusable code will get enhanced in the future and all checkers will benefit from it.

64 ↗

(On Diff #38740)

Ah, right!

209 ↗

(On Diff #38740)

Using regex in a diagnostic is not user friendly. See the comment below.

227 ↗

(On Diff #38740)

It is very important to keep the diagnostics as succinct as possible.

The diagnostics in this checker are long as is and I do not think the "except" clauses are helping the user to understand or fix the problem. They will not get the warning in the cases that are described in the "except" clause so why are we talking about this..

Most likely you are using it to make sure that the message is true. How about rephrasing the message to address that issue. For example, you could use something like:
"Assigning variables (except return value of vfork) is prohibited after a successful fork"
->
"This assignment is prohibited after a successful vfork"

[ygribov]

Updated after review.

[ygribov]

lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

118 ↗

(On Diff #38926)

Semantics is slightly changed for assignment case here: originally S would have been changed iff Init is not NULL but now I also require valid VarDecl on LHS. Frankly I'm not sure if it's a serious change (probably not).

[zaks.anna]

Thanks for addressing all the comments. A couple of additional ones below.

test/Analysis/vfork.c

60 ↗

(On Diff #38926)

Shouldn't the warning appear here as well?

You need to register for ReturnStmt callback instead of the checkEndFunction.

64 ↗

(On Diff #38926)

I think "from this function" is redundant in this particular error message.
"Return from this function is prohibited after a successful vfork" -> "Return is prohibited after a successful vfork; call _exit() instead."

[ygribov]

Updated warning messages.

[zaks.anna]

What is needed to turn this into a non-alpha checker?

I guess that's question for maintainers) We have to consider that vfork is used rarely enough.

Please, make this into a non-alpha, turned on by default.

Thank you!
Anna.

[ygribov]

Moved to unix package (and thus enabled by default). I now also test for collaboration with security.InsecureAPI.vfork.

[ygribov]

I now also test for collaboration with security.InsecureAPI.vfork.

Should probably clarify: I've added checks to testcase to verify that new checker properly interacts with (existing) InsecureAPI.vfork checker,

[zaks.anna]

Ok. Thanks!
I'll commit shortly.

[zaks.anna]

Sounds good!

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h

 //  This file defines CheckerVisitor.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H
 #define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_CHECKERHELPERS_H
 
 #include "clang/AST/Stmt.h"
+#include <tuple>
 
 namespace clang {
 
+class Expr;
+class VarDecl;
+
 namespace ento {
 
 bool containsMacro(const Stmt *S);
 bool containsEnum(const Stmt *S);
 bool containsStaticLocal(const Stmt *S);
 bool containsBuiltinOffsetOf(const Stmt *S);
 template <class T> bool containsStmt(const Stmt *S) {
   if (isa<T>(S))
       return true;
 
   for (const Stmt *Child : S->children())
     if (Child && containsStmt<T>(Child))
       return true;
 
   return false;
 }
 
+std::pair<const clang::VarDecl *, const clang::Expr *>
+parseAssignment(const Stmt *S);
+
 } // end GR namespace
 
 } // end clang namespace
 
 #endif

cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   TraversalChecker.cpp
   UndefBranchChecker.cpp
   UndefCapturedBlockVarChecker.cpp
   UndefResultChecker.cpp
   UndefinedArraySubscriptChecker.cpp
   UndefinedAssignmentChecker.cpp
   UnixAPIChecker.cpp
   UnreachableCodeChecker.cpp
+  VforkChecker.cpp
   VLASizeChecker.cpp
   VirtualCallChecker.cpp
 
   DEPENDS
   ClangSACheckers
 
   LINK_LIBS
   clangAST
   clangAnalysis
   clangBasic
   clangLex
   clangStaticAnalyzerCore
   )

cfe/trunk/lib/StaticAnalyzer/Checkers/Checkers.td

 def MallocSizeofChecker : Checker<"MallocSizeof">,
   HelpText<"Check for dubious malloc arguments involving sizeof">,
   DescFile<"MallocSizeofChecker.cpp">;
 
 def MismatchedDeallocatorChecker : Checker<"MismatchedDeallocator">,
   HelpText<"Check for mismatched deallocators.">,
   DescFile<"MallocChecker.cpp">;
 
+def VforkChecker : Checker<"Vfork">,
+  HelpText<"Check for proper usage of vfork">,
+  DescFile<"VforkChecker.cpp">;
+
 } // end "unix"
 
 let ParentPackage = UnixAlpha in {
 
 def ChrootChecker : Checker<"Chroot">,
   HelpText<"Check improper use of chroot">,
   DescFile<"ChrootChecker.cpp">;
 

cfe/trunk/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

 
 #include "ClangSACheckers.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class DereferenceChecker
void DereferenceChecker::reportBug(ProgramStateRef State, const Stmt *S,
   SmallVector<SourceRange, 2> Ranges;
 
   // Walk through lvalue casts to get the original expression
   // that syntactically caused the load.
   if (const Expr *expr = dyn_cast<Expr>(S))
     S = expr->IgnoreParenLValueCasts();
 
   if (IsBind) {
-    if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(S)) {
-      if (BO->isAssignmentOp())
-        S = BO->getRHS();
-    } else if (const DeclStmt *DS = dyn_cast<DeclStmt>(S)) {
-      assert(DS->isSingleDecl() && "We process decls one by one");
-      if (const VarDecl *VD = dyn_cast<VarDecl>(DS->getSingleDecl()))
-        if (const Expr *Init = VD->getAnyInitializer())
-          S = Init;
-    }
-  }
+    const VarDecl *VD;
+    const Expr *Init;
+    std::tie(VD, Init) = parseAssignment(S);
+    if (VD && Init)
 
   switch (S->getStmtClass()) {
   case Stmt::ArraySubscriptExprClass: {
     os << "Array access";
     const ArraySubscriptExpr *AE = cast<ArraySubscriptExpr>(S);
     AddDerefSource(os, Ranges, AE->getBase()->IgnoreParenCasts(),
                    State.get(), N->getLocationContext());
     os << " results in a null pointer dereference";

cfe/trunk/lib/StaticAnalyzer/Checkers/VforkChecker.cpp

+//===- VforkChecker.cpp -------- Vfork usage checks --------------*- C++ -*-==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines vfork checker which checks for dangerous uses of vfork.
+//  Vforked process shares memory (including stack) with parent so it's
+//  range of actions is significantly limited: can't write variables,
+//  can't call functions not in whitelist, etc. For more details, see
+//  http://man7.org/linux/man-pages/man2/vfork.2.html
+//
+//  This checker checks for prohibited constructs in vforked process.
+//  The state transition diagram:
+//  PARENT ---(vfork() == 0)--> CHILD
+//                                   |
+//                                   --(*p = ...)--> bug
+//                                   |
+//                                   --foo()--> bug
+//                                   |
+//                                   --return--> bug
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/AST/ParentMap.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class VforkChecker : public Checker<check::PreCall, check::PostCall,
+                                    check::Bind, check::PreStmt<ReturnStmt>> {
+  mutable std::unique_ptr<BuiltinBug> BT;
+  mutable llvm::SmallSet<const IdentifierInfo *, 10> VforkWhitelist;
+  mutable const IdentifierInfo *II_vfork;
+
+  static bool isChildProcess(const ProgramStateRef State);
+
+  bool isVforkCall(const Decl *D, CheckerContext &C) const;
+  bool isCallWhitelisted(const IdentifierInfo *II, CheckerContext &C) const;
+
+  void reportBug(const char *What, CheckerContext &C,
+                 const char *Details = 0) const;
+
+public:
+  VforkChecker() : II_vfork(0) {}
+
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkBind(SVal L, SVal V, const Stmt *S, CheckerContext &C) const;
+  void checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const;
+};
+
+} // end anonymous namespace
+
+// This trait holds region of variable that is assigned with vfork's
+// return value (this is the only region child is allowed to write).
+// VFORK_RESULT_INVALID means that we are in parent process.
+// VFORK_RESULT_NONE means that vfork's return value hasn't been assigned.
+// Other values point to valid regions.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(VforkResultRegion, const void *)
+#define VFORK_RESULT_INVALID 0
+#define VFORK_RESULT_NONE ((void *)(uintptr_t)1)
+
+bool VforkChecker::isChildProcess(const ProgramStateRef State) {
+  return State->get<VforkResultRegion>() != VFORK_RESULT_INVALID;
+}
+
+bool VforkChecker::isVforkCall(const Decl *D, CheckerContext &C) const {
+  auto FD = dyn_cast_or_null<FunctionDecl>(D);
+  if (!FD || !C.isCLibraryFunction(FD))
+    return false;
+
+  if (!II_vfork) {
+    ASTContext &AC = C.getASTContext();
+    II_vfork = &AC.Idents.get("vfork");
+  }
+
+  return FD->getIdentifier() == II_vfork;
+}
+
+// Returns true iff ok to call function after successful vfork.
+bool VforkChecker::isCallWhitelisted(const IdentifierInfo *II,
+                                 CheckerContext &C) const {
+  if (VforkWhitelist.empty()) {
+    // According to manpage.
+    const char *ids[] = {
+      "_exit",
+      "_Exit",
+      "execl",
+      "execlp",
+      "execle",
+      "execv",
+      "execvp",
+      "execvpe",
+      0,
+    };
+
+    ASTContext &AC = C.getASTContext();
+    for (const char **id = ids; *id; ++id)
+      VforkWhitelist.insert(&AC.Idents.get(*id));
+  }
+
+  return VforkWhitelist.count(II);
+}
+
+void VforkChecker::reportBug(const char *What, CheckerContext &C,
+                             const char *Details) const {
+  if (ExplodedNode *N = C.generateErrorNode(C.getState())) {
+    if (!BT)
+      BT.reset(new BuiltinBug(this,
+                              "Dangerous construct in a vforked process"));
+
+    SmallString<256> buf;
+    llvm::raw_svector_ostream os(buf);
+
+    os << What << " is prohibited after a successful vfork";
+
+    if (Details)
+      os << "; " << Details;
+
+    auto Report = llvm::make_unique<BugReport>(*BT, os.str(), N);
+    // TODO: mark vfork call in BugReportVisitor
+    C.emitReport(std::move(Report));
+  }
+}
+
+// Detect calls to vfork and split execution appropriately.
+void VforkChecker::checkPostCall(const CallEvent &Call,
+                                 CheckerContext &C) const {
+  // We can't call vfork in child so don't bother
+  // (corresponding warning has already been emitted in checkPreCall).
+  ProgramStateRef State = C.getState();
+  if (isChildProcess(State))
+    return;
+
+  if (!isVforkCall(Call.getDecl(), C))
+    return;
+
+  // Get return value of vfork.
+  SVal VforkRetVal = Call.getReturnValue();
+  Optional<DefinedOrUnknownSVal> DVal =
+    VforkRetVal.getAs<DefinedOrUnknownSVal>();
+  if (!DVal)
+    return;
+
+  // Get assigned variable.
+  const ParentMap &PM = C.getLocationContext()->getParentMap();
+  const Stmt *P = PM.getParentIgnoreParenCasts(Call.getOriginExpr());
+  const VarDecl *LhsDecl;
+  std::tie(LhsDecl, std::ignore) = parseAssignment(P);
+
+  // Get assigned memory region.
+  MemRegionManager &M = C.getStoreManager().getRegionManager();
+  const MemRegion *LhsDeclReg =
+    LhsDecl
+      ? M.getVarRegion(LhsDecl, C.getLocationContext())
+      : (const MemRegion *)VFORK_RESULT_NONE;
+
+  // Parent branch gets nonzero return value (according to manpage).
+  ProgramStateRef ParentState, ChildState;
+  std::tie(ParentState, ChildState) = C.getState()->assume(*DVal);
+  C.addTransition(ParentState);
+  ChildState = ChildState->set<VforkResultRegion>(LhsDeclReg);
+  C.addTransition(ChildState);
+}
+
+// Prohibit calls to non-whitelist functions in child process.
+void VforkChecker::checkPreCall(const CallEvent &Call,
+                                CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  if (isChildProcess(State)
+      && !isCallWhitelisted(Call.getCalleeIdentifier(), C))
+    reportBug("This function call", C);
+}
+
+// Prohibit writes in child process (except for vfork's lhs).
+void VforkChecker::checkBind(SVal L, SVal V, const Stmt *S,
+                             CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  if (!isChildProcess(State))
+    return;
+
+  const MemRegion *VforkLhs =
+    static_cast<const MemRegion *>(State->get<VforkResultRegion>());
+  const MemRegion *MR = L.getAsRegion();
+
+  // Child is allowed to modify only vfork's lhs.
+  if (!MR || MR == VforkLhs)
+    return;
+
+  reportBug("This assignment", C);
+}
+
+// Prohibit return from function in child process.
+void VforkChecker::checkPreStmt(const ReturnStmt *RS, CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  if (isChildProcess(State))
+    reportBug("Return", C, "call _exit() instead");
+}
+
+void ento::registerVforkChecker(CheckerManager &mgr) {
+  mgr.registerChecker<VforkChecker>();
+}

cfe/trunk/lib/StaticAnalyzer/Core/CheckerHelpers.cpp

 //===---- CheckerHelpers.cpp - Helper functions for checkers ----*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines several static functions for use in checkers.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
+#include "clang/AST/Decl.h"
 #include "clang/AST/Expr.h"
 
 // Recursively find any substatements containing macros
 bool clang::ento::containsMacro(const Stmt *S) {
   if (S->getLocStart().isMacroID())
     return true;
 
   if (S->getLocEnd().isMacroID())
if (isa<OffsetOfExpr>(S))
     return true;
 
   for (const Stmt *Child : S->children())
     if (Child && containsBuiltinOffsetOf(Child))
       return true;
 
   return false;
 }
+
+// Extract lhs and rhs from assignment statement
+std::pair<const clang::VarDecl *, const clang::Expr *>
+clang::ento::parseAssignment(const Stmt *S) {
+  const VarDecl *VD = 0;
+  const Expr *RHS = 0;
+
+  if (auto Assign = dyn_cast_or_null<BinaryOperator>(S)) {
+    if (Assign->isAssignmentOp()) {
+      // Ordinary assignment
+      RHS = Assign->getRHS();
+      if (auto DE = dyn_cast_or_null<DeclRefExpr>(Assign->getLHS()))
+        VD = dyn_cast_or_null<VarDecl>(DE->getDecl());
+    }
+  } else if (auto PD = dyn_cast_or_null<DeclStmt>(S)) {
+    // Initialization
+    assert(PD->isSingleDecl() && "We process decls one by one");
+    VD = dyn_cast_or_null<VarDecl>(PD->getSingleDecl());
+    RHS = VD->getAnyInitializer();
+  }
+
+  return std::make_pair(VD, RHS);
+}

cfe/trunk/test/Analysis/Inputs/system-header-simulator.h

 typedef struct fake_rb_tree { void *opaque[8]; } fake_rb_tree_t;
 void fake_rb_tree_init(fake_rb_tree_t *, const void *);
 void *fake_rb_tree_insert_node(fake_rb_tree_t *, void *);
 
 typedef struct __SomeStruct {
   char * p;
 } SomeStruct;
 void fakeSystemHeaderCall(SomeStruct *);
+
+typedef int pid_t;
+pid_t fork(void);
+pid_t vfork(void);
+int execl(const char *path, const char *arg, ...);
+
+void exit(int status) __attribute__ ((__noreturn__));
+void _exit(int status) __attribute__ ((__noreturn__));
+void _Exit(int status) __attribute__ ((__noreturn__));
+

cfe/trunk/test/Analysis/vfork.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,security.insecureAPI.vfork,unix.Vfork -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,security.insecureAPI.vfork,unix.Vfork -verify -x c++ %s
+
+#include "Inputs/system-header-simulator.h"
+
+void foo();
+
+// Ensure that child process is properly checked.
+int f1(int x) {
+  pid_t pid = vfork(); // expected-warning{{Call to function 'vfork' is insecure}}
+  if (pid != 0)
+    return 0;
+
+  switch (x) {
+  case 0:
+    // Ensure that modifying pid is ok.
+    pid = 1; // no-warning
+    // Ensure that calling whitelisted routines is ok.
+    execl("", "", 0); // no-warning
+    _exit(1); // no-warning
+    break;
+  case 1:
+    // Ensure that writing variables is prohibited.
+    x = 0; // expected-warning{{This assignment is prohibited after a successful vfork}}
+    break;
+  case 2:
+    // Ensure that calling functions is prohibited.
+    foo(); // expected-warning{{This function call is prohibited after a successful vfork}}
+    break;
+  default:
+    // Ensure that returning from function is prohibited.
+    return 0; // expected-warning{{Return is prohibited after a successful vfork; call _exit() instead}}
+  }
+
+  while(1);
+}
+
+// Same as previous but without explicit pid variable.
+int f2(int x) {
+  pid_t pid = vfork(); // expected-warning{{Call to function 'vfork' is insecure}}
+
+  switch (x) {
+  case 0:
+    // Ensure that writing pid is ok.
+    pid = 1; // no-warning
+    // Ensure that calling whitelisted routines is ok.
+    execl("", "", 0); // no-warning
+    _exit(1); // no-warning
+    break;
+  case 1:
+    // Ensure that writing variables is prohibited.
+    x = 0; // expected-warning{{This assignment is prohibited after a successful vfork}}
+    break;
+  case 2:
+    // Ensure that calling functions is prohibited.
+    foo(); // expected-warning{{This function call is prohibited after a successful vfork}}
+    break;
+  default:
+    // Ensure that returning from function is prohibited.
+    return 0; // expected-warning{{Return is prohibited after a successful vfork; call _exit() instead}}
+  }
+
+  while(1);
+}
+
+// Ensure that parent process isn't restricted.
+int f3(int x) {
+  if (vfork() == 0) // expected-warning{{Call to function 'vfork' is insecure}}
+    _exit(1);
+  x = 0; // no-warning
+  foo(); // no-warning
+  return 0;
+} // no-warning
+
+// Unbound pids are special so test them separately.
+void f4(int x) {
+  switch (x) {
+  case 0:
+    vfork(); // expected-warning{{Call to function 'vfork' is insecure}}
+    x = 0; // expected-warning{{This assignment is prohibited after a successful vfork}}
+    break;
+
+  case 1:
+    {
+      char args[2];
+      switch (vfork()) { // expected-warning{{Call to function 'vfork' is insecure}}
+      case 0:
+        args[0] = 0; // expected-warning{{This assignment is prohibited after a successful vfork}}
+        exit(1);
+      }
+      break;
+    }
+
+  case 2:
+    {
+      pid_t pid;
+      if ((pid = vfork()) == 0) // expected-warning{{Call to function 'vfork' is insecure}}
+        while(1); // no-warning
+      break;
+    }
+  }
+  while(1);
+} //no-warning
+
+
+void f5() {
+  // See "libxtables: move some code to avoid cautions in vfork man page"
+  // (http://lists.netfilter.org/pipermail/netfilter-buglog/2014-October/003280.html).
+  if (vfork() == 0) { // expected-warning{{Call to function 'vfork' is insecure}}
+    execl("prog", "arg1", 0); // no-warning
+    exit(1);  // expected-warning{{This function call is prohibited after a successful vfork}}
+  }
+}
+