[analyzer] Valist checkers.

[xazax.hun]

Summary

This patch introduces two new checks for incorrect usage of va_lists. They finds usages of uninitialized va_lists and leaked/unterminated va_lists. They can also find va_lists that are copied into itself.

This check was developed by Donat Nagy ([e-mail]) and myself.

[zaks.anna]

Looks good overall; comments below.

Please, provide more information on real world code evaluation. Which codebases this has been tested on? What was the false positive rate? How many real bugs were found/fixed?

What is the criteria for taking it out of alpha?

Please, write the commit message that will be used.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

30 ↗

(On Diff #41865)

This looks like a general purpose utility that could be used by other checkers.
(Though, this code might not support all languages as is.)

54 ↗

(On Diff #41865)

What are "likes" ?

82 ↗

(On Diff #41865)

Maybe these could be called "checkVAListEndCall" or something similar; as is they are too similar to "checkPreCall/checkPostCall".

116 ↗

(On Diff #41865)

Explain what's in the pair or make it into a struct. (I suspect the second argument is the index of the argument taking the va_arg.)

204 ↗

(On Diff #41865)

This should be a general-purpose utility. It's not a good idea for each checker to roll their own implementation:) See 'variableName' in http://reviews.llvm.org/D12761.

There could be other places in the analyzer where we print the names.

217 ↗

(On Diff #41865)

Please, add a comment explaining what this does and roughly how.

Looks like the main difference with Malloc checker's getAllocationSite is that this does not look for the ReferenceRegion. That might not matter for va lists since the usage is not likely to span many inlined functions, is that correct?

361 ↗

(On Diff #41865)

Please add tests for this, preferably the plist output.

test/Analysis/Inputs/system-header-simulator-for-valist.h

4 ↗

(On Diff #41865)

Does the second part of the comment refer to something you test for here?

test/Analysis/Inputs/system-header-simulator-valist.h

3 ↗

(On Diff #41865)

Is this an unused duplicate file?

test/Analysis/valist-uninitialized.c

64 ↗

(On Diff #41865)

http://llvm.org/docs/CodingStandards.html#commenting

98 ↗

(On Diff #41865)

The majority of functions end with "//no-warning" comment. Why is that?

134 ↗

(On Diff #41865)

Is this something we would want to warn on or not?

146 ↗

(On Diff #41865)

You could call this "inlined_uses_arg".

[xazax.hun]

It was tested on gcc and rAthena (https://github.com/rathena/rathena). It did not find any issues in those projects, but I was able to find some issues that I artificially put into those projects.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

30 ↗

(On Diff #41865)

I agree.
Right now it does not support namespaces, overloading on the types of the argument, overloading on CV qualifiers etc. Do you have any other language features that is not supported in mind?
Supporting all of that might be useful but I am a bit afraid of the result being bloated. What do you think, what would be the most appropriate place to put such utility? It could be a helper class in CallEvent.cpp/h.

54 ↗

(On Diff #41865)

va_list accepting functions.

82 ↗

(On Diff #41865)

You are right, I will rename this to better reflect their role.

204 ↗

(On Diff #41865)

I agree that it would be great to have this as a general utility. I think I should wait until the implementation from the other checker gets commited.

217 ↗

(On Diff #41865)

Right. Clarified this in the comment.

test/Analysis/Inputs/system-header-simulator-for-valist.h

4 ↗

(On Diff #41865)

Added a test for it.

test/Analysis/Inputs/system-header-simulator-valist.h

3 ↗

(On Diff #41865)

Yes, it is.

test/Analysis/valist-uninitialized.c

98 ↗

(On Diff #41865)

To indicate that, no leak related warning there. Do you prefer to remove those comments?

134 ↗

(On Diff #41865)

We should definitely wanr on that, but I think it should be ok to add this feature later.

[xazax.hun]

• Addressed review comments.
• Updated to latest trunk.

[zaks.anna]

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

31 ↗

(On Diff #43435)

It does not support ObjC methods.

I think this is most useful to checker writes, so how about placing it in CheckerContext? (I am fine with CallEven as well if you think it has wider applicability.)

205 ↗

(On Diff #43435)

How about we commit your implementation as a general utility and point the other review to it? You can make it as a separate commit for clarity.

test/Analysis/valist-uninitialized.c

135 ↗

(On Diff #43435)

Please, make that clear in the comment, for example, you can say "We should produce a warning here..".

[xazax.hun]

• Updated to latest trunk.
• Migrated to use the new CallDescription interface.
• Use http://reviews.llvm.org/D16044 to extract variable name from regions.

[xazax.hun]

• Updated to work properly with http://reviews.llvm.org/D16044

[xazax.hun]

• Updated to latest trunk.
• All prerequisites are now committed.

[dcoughlin]

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

177 ↗

(On Diff #60497)

Are there tests for this diagnostic? I don't see any.

[NoQ]

The checker's in great shape! I see a few minor things, but that's it.

The checks are split into two sections ("uninitialized" and "unterminated"), but there seem to be more auxiliary checks provided (eg. "copies into itself") that are on for both checkers, do you think you need to add more flags to make the checker more flexible, or maybe just merge everything into a single checker?

Because plist tests are so heavy: if all you wanted to test was how your bug reporter visitor works, did you consider using -analyzer-output=text instead? (this is not the default output mode; it has note: diagnostics displaying the path, which you can catch with expected-note{{}}). Also, you could probably write special tests for the visitor, instead of testing the visitors on all tests, regardless of text/plist. But i'm merely sharing this hint, i don't think it's worth rewriting once we already have plist tests.

lib/StaticAnalyzer/Checkers/ValistChecker.cpp

177 ↗

(On Diff #60497)

In the other file, first few tests in valist-unterminated.c.

185 ↗

(On Diff #60497)

At first, i thought that you're trying to strip casts from symbolic pointers here. But then i figured out that you are stripping an implementation detail here - as even VarRegion-based VAs reach here in the form of "element{va,0 S64b,struct __va_list_tag}". I've a feeling this deserves a comment.

254 ↗

(On Diff #60497)

Does static const Stmt *PathDiagnosticLocation::getStmt(const ExplodedNode *) work for you? Because i guess it was specifically designed for that purpose. Here and in one more place where this code is duplicated.

test/Analysis/valist-uninitialized.c

64 ↗

(On Diff #60497)

Trailing whitespace :)

98 ↗

(On Diff #60497)

A bit more trailing whitespace here and below.

178 ↗

(On Diff #60497)

Like the compiler, the static analyzer treats some functions differently if
they come from a system header -- for example, it is assumed that system
functions do not arbitrarily free() their parameters//, and that some bugs
found in system headers cannot be fixed by the user and should be
suppressed.

Perhaps library functions can be assumed to never va_end() va_lists either. The idea behind the trick, which is also used in, say, SimpleStreamChecker, is that we know all library functions, we can list all library functions that va_end() va_lists, so it's safe to assume that all other library functions do not va_end() va_lists (completely unlike other, non-standard functions with unavailable bodies).

So i think this test should warn, and it shouldn't be hard to fix. That said, some checkers (eg. PthreadLockChecker) do not implement this trick, so it's not critical, but it may give you slightly more positives.

test/Analysis/valist-unterminated.c

42 ↗

(On Diff #60497)

Wow, these tests are mind-breaking :))

Is it ok if i ask to put the FIXME above the test or above no-warning or into function name? Because it took some time for me to figure out that f7 is not FIXME, being used to the traditional positioning of the FIXME comment><

[xazax.hun]

• Improvements according to review comments.

[xazax.hun]

It looks like it broke some of the build bots.

Error from the windows build bots:

error: 'note' diagnostics expected but not seen: 
  File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 96: va_list 'va' is copied onto itself
  File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 102: va_list 'va' is copied onto itself
  File C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\tools\clang\test\Analysis\valist-uninitialized.c Line 108: Initialized va_list 'va' is overwritten by an uninitialized one
error: 'note' diagnostics seen but not expected: 
  Line 96: va_list va' is copied onto itself
  Line 102: va_list va' is copied onto itself
  Line 108: Initialized va_list va' is overwritten by an uninitialized one
12 errors generated.

Somehow the beginning single quote is missing from the generated message. It is very strange.

Error from other architectures:

error: 'warning' diagnostics expected but not seen: 
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 27: Initialized va_list is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 39: Initialized va_list is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array[3]' is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem[0]' is leaked
error: 'warning' diagnostics seen but not expected: 
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 33: Initialized va_list 'fst' is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array' is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem' is leaked
error: 'note' diagnostics expected but not seen: 
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 26: Initialized va_list
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 27: Initialized va_list is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 36: Initialized va_list
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 39: Initialized va_list is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array[3]' is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem[0]' is leaked
error: 'note' diagnostics seen but not expected: 
  Line 31: Initialized va_list
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 33: Initialized va_list 'fst' is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 110: Initialized va_list 'va_array' is leaked
  File /var/lib/buildbot/slaves/hexagon-build-03/clang-hexagon-elf/llvm/tools/clang/test/Analysis/valist-unterminated.c Line 125: Initialized va_list 'mem' is leaked
17 errors generated.

I suspect that slightly different AST is generated for those architectures that cause the different behavior. I will further investigate those problems.

[NoQ]

In D15227#519239, @xazax.hun wrote:

I suspect that slightly different AST is generated for those architectures that cause the different behavior. I will further investigate those problems.

Seems so, because on my machine when i append -triple hexagon-unknown-linux to the test run line, a bunch of things start failing. You may also consider hardcoding the runline in the current test and commit (which is so often a good idea), and fix other stuff in later commits. A proper investigation would be great, of course :)

P.S. Just curious - do you plan to eventually model va_arg(), as we discussed with Michael Tandy in http://lists.llvm.org/pipermail/cfe-dev/2016-August/050202.html ?

[xazax.hun]

In D15227#521781, @NoQ wrote:

In D15227#519239, @xazax.hun wrote:

I suspect that slightly different AST is generated for those architectures that cause the different behavior. I will further investigate those problems.

Seems so, because on my machine when i append -triple hexagon-unknown-linux to the test run line, a bunch of things start failing. You may also consider hardcoding the runline in the current test and commit (which is so often a good idea), and fix other stuff in later commits. A proper investigation would be great, of course :)

I committed with the hardcoded triple in the runline for now.

P.S. Just curious - do you plan to eventually model va_arg(), as we discussed with Michael Tandy in http://lists.llvm.org/pipermail/cfe-dev/2016-August/050202.html ?

It is not on my TODO list at the moment, but I might look into it in the distant future.

[zaks.anna]

@xazax.hun,

Can we move this out of alpha?

Have this checkers been tested on a large codebase? What are false positive rates?

Thanks!
Anna

[xazax.hun]

In D15227#674278, @zaks.anna wrote:

@xazax.hun,

Can we move this out of alpha?

Have this checkers been tested on a large codebase? What are false positive rates?

I have tested it on a few ~200k LOC C codebase and I did not see any major problem. There is one problem left though, when I committed this check some of the build bots broke, I suspect that slightly different AST is generated on some of the platforms. I temporarily fixed the issue by hardcoding the triple into the tests and I did not have time yet to investigate the source of the problems. But as far as I remember, this produced false negatives in the tests not false positives.

[zaks.anna]

But as far as I remember, this produced false negatives in the tests not false positives.

Could you double check that? Maybe you still have some notes in your mail box or just by looking at the code.

Did none of the checks work or just some of them?

Also, which platforms did this not work on?

It would be great to move all of the useful checks out of alpha since alpha essentially means "unsupported" and we do not recommend turning those checkers on.

Thanks!

[xazax.hun]

In D15227#681127, @zaks.anna wrote:

But as far as I remember, this produced false negatives in the tests not false positives.

Could you double check that? Maybe you still have some notes in your mail box or just by looking at the code.

Did none of the checks work or just some of them?

Also, which platforms did this not work on?

It would be great to move all of the useful checks out of alpha since alpha essentially means "unsupported" and we do not recommend turning those checkers on.

Thanks!

I think I managed to solve the problems due to the different AST variants. In the future this should be abstracted away by the analyzer core. You can see the patch here: https://reviews.llvm.org/D30157

Early next week I will rerun this check on a larger open source project. If it does not give me too much false positives or crash, I think it is a good candidate to move out from alpha.

cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td

 // default. Such checkers belong in the alpha package.
 def OptIn : Package<"optin">;
 
 def Nullability : Package<"nullability">;
 
 def Cplusplus : Package<"cplusplus">;
 def CplusplusAlpha : Package<"cplusplus">, InPackage<Alpha>, Hidden;
 
+def Valist : Package<"valist">;
+def ValistAlpha : Package<"valist">, InPackage<Alpha>, Hidden;
+
 def DeadCode : Package<"deadcode">;
 def DeadCodeAlpha : Package<"deadcode">, InPackage<Alpha>, Hidden;
 
 def Performance : Package<"performance">, InPackage<OptIn>;
 
 def Security : Package <"security">;
 def InsecureAPI : Package<"insecureAPI">, InPackage<Security>;
 def SecurityAlpha : Package<"security">, InPackage<Alpha>, Hidden;

 let ParentPackage = CplusplusAlpha in {
 
 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction or destruction">,
   DescFile<"VirtualCallChecker.cpp">;
 
 } // end: "alpha.cplusplus"
 
+
+//===----------------------------------------------------------------------===//
+// Valist checkers.
+//===----------------------------------------------------------------------===//
+
+let ParentPackage = ValistAlpha in {
+
+def UninitializedChecker : Checker<"Uninitialized">,
+  HelpText<"Check for usages of uninitialized (or already released) va_lists.">,
+  DescFile<"ValistChecker.cpp">;
+
+def UnterminatedChecker : Checker<"Unterminated">,
+  HelpText<"Check for va_lists which are not released by a va_end call.">,
+  DescFile<"ValistChecker.cpp">;
+
+def CopyToSelfChecker : Checker<"CopyToSelf">,
+  HelpText<"Check for va_lists which are copied onto itself.">,
+  DescFile<"ValistChecker.cpp">;
+
+} // end : "alpha.valist"
+
 //===----------------------------------------------------------------------===//
 // Deadcode checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = DeadCode in {
 
 def DeadStoresChecker : Checker<"DeadStores">,
   HelpText<"Check for values stored to variables that are never read afterwards">,

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

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

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

+//== ValistChecker.cpp - stdarg.h macro usage checker -----------*- C++ -*--==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines checkers which detect usage of uninitialized va_list values
+// and va_start calls with no matching va_end.
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.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/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+REGISTER_SET_WITH_PROGRAMSTATE(InitializedVALists, const MemRegion *)
+
+namespace {
+typedef SmallVector<const MemRegion *, 2> RegionVector;
+
+class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
+                                     check::DeadSymbols> {
+  mutable std::unique_ptr<BugType> BT_leakedvalist, BT_uninitaccess;
+
+  struct VAListAccepter {
+    CallDescription Func;
+    int VAListPos;
+  };
+  static const SmallVector<VAListAccepter, 15> VAListAccepters;
+  static const CallDescription VaStart, VaEnd, VaCopy;
+
+public:
+  enum CheckKind {
+    CK_Uninitialized,
+    CK_Unterminated,
+    CK_CopyToSelf,
+    CK_NumCheckKinds
+  };
+
+  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+  CheckName CheckNames[CK_NumCheckKinds];
+
+  void checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const;
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+  void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
+
+private:
+  const MemRegion *getVAListAsRegion(SVal SV, CheckerContext &C) const;
+  StringRef getVariableNameFromRegion(const MemRegion *Reg) const;
+  const ExplodedNode *getStartCallSite(const ExplodedNode *N,
+                                       const MemRegion *Reg,
+                                       CheckerContext &C) const;
+
+  void reportUninitializedAccess(const MemRegion *VAList, StringRef Msg,
+                                 CheckerContext &C) const;
+  void reportLeakedVALists(const RegionVector &LeakedVALists, StringRef Msg1,
+                           StringRef Msg2, CheckerContext &C, ExplodedNode *N,
+                           bool ForceReport = false) const;
+
+  void checkVAListStartCall(const CallEvent &Call, CheckerContext &C,
+                            bool IsCopy) const;
+  void checkVAListEndCall(const CallEvent &Call, CheckerContext &C) const;
+
+  class ValistBugVisitor : public BugReporterVisitorImpl<ValistBugVisitor> {
+  public:
+    ValistBugVisitor(const MemRegion *Reg, bool IsLeak = false)
+        : Reg(Reg), IsLeak(IsLeak) {}
+    void Profile(llvm::FoldingSetNodeID &ID) const override {
+      static int X = 0;
+      ID.AddPointer(&X);
+      ID.AddPointer(Reg);
+    }
+    std::unique_ptr<PathDiagnosticPiece>
+    getEndPath(BugReporterContext &BRC, const ExplodedNode *EndPathNode,
+               BugReport &BR) override {
+      if (!IsLeak)
+        return nullptr;
+
+      PathDiagnosticLocation L = PathDiagnosticLocation::createEndOfPath(
+          EndPathNode, BRC.getSourceManager());
+      // Do not add the statement itself as a range in case of leak.
+      return llvm::make_unique<PathDiagnosticEventPiece>(L, BR.getDescription(),
+                                                         false);
+    }
+    PathDiagnosticPiece *VisitNode(const ExplodedNode *N,
+                                   const ExplodedNode *PrevN,
+                                   BugReporterContext &BRC,
+                                   BugReport &BR) override;
+
+  private:
+    const MemRegion *Reg;
+    bool IsLeak;
+  };
+};
+
+const SmallVector<ValistChecker::VAListAccepter, 15>
+    ValistChecker::VAListAccepters = {
+        {{"vfprintf", 3}, 2},
+        {{"vfscanf", 3}, 2},
+        {{"vprintf", 2}, 1},
+        {{"vscanf", 2}, 1},
+        {{"vsnprintf", 4}, 3},
+        {{"vsprintf", 3}, 2},
+        {{"vsscanf", 3}, 2},
+        {{"vfwprintf", 3}, 2},
+        {{"vfwscanf", 3}, 2},
+        {{"vwprintf", 2}, 1},
+        {{"vwscanf", 2}, 1},
+        {{"vswprintf", 4}, 3},
+        // vswprintf is the wide version of vsnprintf,
+        // vsprintf has no wide version
+        {{"vswscanf", 3}, 2}};
+const CallDescription ValistChecker::VaStart("__builtin_va_start", 2),
+    ValistChecker::VaCopy("__builtin_va_copy", 2),
+    ValistChecker::VaEnd("__builtin_va_end", 1);
+} // end anonymous namespace
+
+void ValistChecker::checkPreCall(const CallEvent &Call,
+                                 CheckerContext &C) const {
+  if (!Call.isGlobalCFunction())
+    return;
+  if (Call.isCalled(VaStart))
+    checkVAListStartCall(Call, C, false);
+  else if (Call.isCalled(VaCopy))
+    checkVAListStartCall(Call, C, true);
+  else if (Call.isCalled(VaEnd))
+    checkVAListEndCall(Call, C);
+  else {
+    for (auto FuncInfo : VAListAccepters) {
+      if (!Call.isCalled(FuncInfo.Func))
+        continue;
+      const MemRegion *VAList =
+          getVAListAsRegion(Call.getArgSVal(FuncInfo.VAListPos), C);
+      if (!VAList)
+        return;
+
+      if (C.getState()->contains<InitializedVALists>(VAList))
+        return;
+
+      SmallString<80> Errmsg("Function '");
+      Errmsg += FuncInfo.Func.getFunctionName();
+      Errmsg += "' is called with an uninitialized va_list argument";
+      reportUninitializedAccess(VAList, Errmsg.c_str(), C);
+      break;
+    }
+  }
+}
+
+void ValistChecker::checkPreStmt(const VAArgExpr *VAA,
+                                 CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  SVal VAListSVal = State->getSVal(VAA->getSubExpr(), C.getLocationContext());
+  const MemRegion *VAList = getVAListAsRegion(VAListSVal, C);
+  if (!VAList)
+    return;
+  if (!State->contains<InitializedVALists>(VAList))
+    reportUninitializedAccess(
+        VAList, "va_arg() is called on an uninitialized va_list", C);
+}
+
+void ValistChecker::checkDeadSymbols(SymbolReaper &SR,
+                                     CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  InitializedVAListsTy TrackedVALists = State->get<InitializedVALists>();
+  RegionVector LeakedVALists;
+  for (auto Reg : TrackedVALists) {
+    if (SR.isLiveRegion(Reg))
+      continue;
+    LeakedVALists.push_back(Reg);
+    State = State->remove<InitializedVALists>(Reg);
+  }
+  if (ExplodedNode *N = C.addTransition(State))
+    reportLeakedVALists(LeakedVALists, "Initialized va_list", " is leaked", C,
+                        N);
+}
+
+const MemRegion *ValistChecker::getVAListAsRegion(SVal SV,
+                                                  CheckerContext &C) const {
+  const MemRegion *Reg = SV.getAsRegion();
+  const auto *TReg = dyn_cast_or_null<TypedValueRegion>(Reg);
+  // Some VarRegion based VLAs reach here as ElementRegions.
+  const auto *EReg = dyn_cast_or_null<ElementRegion>(TReg);
+  return EReg ? EReg->getSuperRegion() : TReg;
+}
+
+// This function traverses the exploded graph backwards and finds the node where
+// the va_list is initialized. That node is used for uniquing the bug paths.
+// It is not likely that there are several different va_lists that belongs to
+// different stack frames, so that case is not yet handled.
+const ExplodedNode *ValistChecker::getStartCallSite(const ExplodedNode *N,
+                                                    const MemRegion *Reg,
+                                                    CheckerContext &C) const {
+  const LocationContext *LeakContext = N->getLocationContext();
+  const ExplodedNode *StartCallNode = N;
+
+  bool FoundInitializedState = false;
+
+  while (N) {
+    ProgramStateRef State = N->getState();
+    if (!State->contains<InitializedVALists>(Reg)) {
+      if (FoundInitializedState)
+        break;
+    } else {
+      FoundInitializedState = true;
+    }
+    const LocationContext *NContext = N->getLocationContext();
+    if (NContext == LeakContext || NContext->isParentOf(LeakContext))
+      StartCallNode = N;
+    N = N->pred_empty() ? nullptr : *(N->pred_begin());
+  }
+
+  return StartCallNode;
+}
+
+void ValistChecker::reportUninitializedAccess(const MemRegion *VAList,
+                                              StringRef Msg,
+                                              CheckerContext &C) const {
+  if (!ChecksEnabled[CK_Uninitialized])
+    return;
+  if (ExplodedNode *N = C.generateErrorNode()) {
+    if (!BT_uninitaccess)
+      BT_uninitaccess.reset(new BugType(CheckNames[CK_Uninitialized],
+                                        "Uninitialized va_list",
+                                        "Memory Error"));
+    auto R = llvm::make_unique<BugReport>(*BT_uninitaccess, Msg, N);
+    R->markInteresting(VAList);
+    R->addVisitor(llvm::make_unique<ValistBugVisitor>(VAList));
+    C.emitReport(std::move(R));
+  }
+}
+
+void ValistChecker::reportLeakedVALists(const RegionVector &LeakedVALists,
+                                        StringRef Msg1, StringRef Msg2,
+                                        CheckerContext &C, ExplodedNode *N,
+                                        bool ForceReport) const {
+  if (!(ChecksEnabled[CK_Unterminated] ||
+        (ChecksEnabled[CK_Uninitialized] && ForceReport)))
+    return;
+  for (auto Reg : LeakedVALists) {
+    if (!BT_leakedvalist) {
+      BT_leakedvalist.reset(new BugType(CheckNames[CK_Unterminated],
+                                        "Leaked va_list", "Memory Error"));
+      BT_leakedvalist->setSuppressOnSink(true);
+    }
+
+    const ExplodedNode *StartNode = getStartCallSite(N, Reg, C);
+    PathDiagnosticLocation LocUsedForUniqueing;
+
+    if (const Stmt *StartCallStmt = PathDiagnosticLocation::getStmt(StartNode))
+      LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
+          StartCallStmt, C.getSourceManager(), StartNode->getLocationContext());
+
+    SmallString<100> Buf;
+    llvm::raw_svector_ostream OS(Buf);
+    OS << Msg1;
+    StringRef VariableName = Reg->getDescriptiveName();
+    if (!VariableName.empty())
+      OS << " " << VariableName;
+    OS << Msg2;
+
+    auto R = llvm::make_unique<BugReport>(
+        *BT_leakedvalist, OS.str(), N, LocUsedForUniqueing,
+        StartNode->getLocationContext()->getDecl());
+    R->markInteresting(Reg);
+    R->addVisitor(llvm::make_unique<ValistBugVisitor>(Reg, true));
+    C.emitReport(std::move(R));
+  }
+}
+
+void ValistChecker::checkVAListStartCall(const CallEvent &Call,
+                                         CheckerContext &C, bool IsCopy) const {
+  const MemRegion *VAList = getVAListAsRegion(Call.getArgSVal(0), C);
+  ProgramStateRef State = C.getState();
+  if (!VAList)
+    return;
+
+  if (IsCopy) {
+    const MemRegion *Arg2 = getVAListAsRegion(Call.getArgSVal(1), C);
+    if (Arg2) {
+      if (ChecksEnabled[CK_CopyToSelf] && VAList == Arg2) {
+        RegionVector LeakedVALists{VAList};
+        if (ExplodedNode *N = C.addTransition(State))
+          reportLeakedVALists(LeakedVALists, "va_list",
+                              " is copied onto itself", C, N, true);
+        return;
+      } else if (!State->contains<InitializedVALists>(Arg2)) {
+        if (State->contains<InitializedVALists>(VAList)) {
+          State = State->remove<InitializedVALists>(VAList);
+          RegionVector LeakedVALists{VAList};
+          if (ExplodedNode *N = C.addTransition(State))
+            reportLeakedVALists(LeakedVALists, "Initialized va_list",
+                                " is overwritten by an uninitialized one", C, N,
+                                true);
+        } else {
+          reportUninitializedAccess(Arg2, "Uninitialized va_list is copied", C);
+        }
+        return;
+      }
+    }
+  }
+  if (State->contains<InitializedVALists>(VAList)) {
+    RegionVector LeakedVALists{VAList};
+    if (ExplodedNode *N = C.addTransition(State))
+      reportLeakedVALists(LeakedVALists, "Initialized va_list",
+                          " is initialized again", C, N);
+    return;
+  }
+
+  State = State->add<InitializedVALists>(VAList);
+  C.addTransition(State);
+}
+
+void ValistChecker::checkVAListEndCall(const CallEvent &Call,
+                                       CheckerContext &C) const {
+  const MemRegion *VAList = getVAListAsRegion(Call.getArgSVal(0), C);
+  if (!VAList)
+    return;
+
+  if (!C.getState()->contains<InitializedVALists>(VAList)) {
+    reportUninitializedAccess(
+        VAList, "va_end() is called on an uninitialized va_list", C);
+    return;
+  }
+  ProgramStateRef State = C.getState();
+  State = State->remove<InitializedVALists>(VAList);
+  C.addTransition(State);
+}
+
+PathDiagnosticPiece *ValistChecker::ValistBugVisitor::VisitNode(
+    const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
+    BugReport &BR) {
+  ProgramStateRef State = N->getState();
+  ProgramStateRef StatePrev = PrevN->getState();
+
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+    return nullptr;
+
+  StringRef Msg;
+  if (State->contains<InitializedVALists>(Reg) &&
+      !StatePrev->contains<InitializedVALists>(Reg))
+    Msg = "Initialized va_list";
+  else if (!State->contains<InitializedVALists>(Reg) &&
+           StatePrev->contains<InitializedVALists>(Reg))
+    Msg = "Ended va_list";
+
+  if (Msg.empty())
+    return nullptr;
+
+  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+                             N->getLocationContext());
+  return new PathDiagnosticEventPiece(Pos, Msg, true);
+}
+
+#define REGISTER_CHECKER(name)                                                 \
+  void ento::register##name##Checker(CheckerManager &mgr) {                    \
+    ValistChecker *checker = mgr.registerChecker<ValistChecker>();             \
+    checker->ChecksEnabled[ValistChecker::CK_##name] = true;                   \
+    checker->CheckNames[ValistChecker::CK_##name] = mgr.getCurrentCheckName(); \
+  }
+
+REGISTER_CHECKER(Uninitialized)
+REGISTER_CHECKER(Unterminated)
+REGISTER_CHECKER(CopyToSelf)

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

+// Like the compiler, the static analyzer treats some functions differently if
+// they come from a system header -- for example, it is assumed that system
+// functions do not arbitrarily free() their parameters, and that some bugs
+// found in system headers cannot be fixed by the user and should be
+// suppressed.
+
+#pragma clang system_header
+
+#ifdef __cplusplus
+#define restrict /*restrict*/
+#endif
+
+typedef __builtin_va_list va_list;
+
+#define va_start(ap, param) __builtin_va_start(ap, param)
+#define va_end(ap)          __builtin_va_end(ap)
+#define va_arg(ap, type)    __builtin_va_arg(ap, type)
+#define va_copy(dst, src)   __builtin_va_copy(dst, src)
+
+int vprintf (const char *restrict format, va_list arg);
+
+int vsprintf (char *restrict s, const char *restrict format, va_list arg);
+
+int some_library_function(int n, va_list arg);
+
+// No warning from system header.
+inline void __impl_detail(int fst, ...) {
+  va_list va;
+  (void)va_arg(va, int);
+}

cfe/trunk/test/Analysis/valist-uninitialized.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.valist.Uninitialized,alpha.valist.CopyToSelf -analyzer-output=text -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void f1(int fst, ...) {
+  va_list va;
+  (void)va_arg(va, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+}
+
+int f2(int fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  va_end(va); // expected-note{{Ended va_list}}
+  return va_arg(va, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+}
+
+void f3(int fst, ...) {
+  va_list va, va2;
+  va_start(va, fst);
+  va_copy(va2, va);
+  va_end(va);
+  (void)va_arg(va2, int);
+  va_end(va2);
+} //no-warning
+
+void f4(int cond, ...) {
+  va_list va;
+  if (cond) { // expected-note{{Assuming 'cond' is 0}} expected-note{{Taking false branch}}
+    va_start(va, cond);
+    (void)va_arg(va,int);
+  }
+  va_end(va); //expected-warning{{va_end() is called on an uninitialized va_list}} expected-note{{va_end() is called on an uninitialized va_list}}
+}
+
+void f5(va_list fst, ...) {
+  va_start(fst, fst);
+  (void)va_arg(fst, int);
+  va_end(fst);
+} // no-warning
+
+//FIXME: this should not cause a warning
+void f6(va_list *fst, ...) {
+  va_start(*fst, fst);
+  (void)va_arg(*fst, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+  va_end(*fst);
+}
+
+void f7(int *fst, ...) {
+  va_list x;
+  va_list *y = &x;
+  va_start(*y,fst);
+  (void)va_arg(x, int);
+  va_end(x);
+} // no-warning
+
+void f8(int *fst, ...) {
+  va_list x;
+  va_list *y = &x;
+  va_start(*y,fst); // expected-note{{Initialized va_list}}
+  va_end(x); // expected-note{{Ended va_list}}
+  (void)va_arg(*y, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+} // no-warning
+
+// This only contains problems which are handled by varargs.Unterminated.
+void reinit(int *fst, ...) {
+  va_list va;
+  va_start(va, fst);
+  va_start(va, fst);
+  (void)va_arg(va, int);
+} // no-warning
+
+void reinitOk(int *fst, ...) {
+  va_list va;
+  va_start(va, fst);
+  (void)va_arg(va, int);
+  va_end(va);
+  va_start(va, fst);
+  (void)va_arg(va, int);
+  va_end(va);
+} // no-warning
+
+void reinit3(int *fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  (void)va_arg(va, int);
+  va_end(va); // expected-note{{Ended va_list}}
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  (void)va_arg(va, int);
+  va_end(va); // expected-note{{Ended va_list}}
+  (void)va_arg(va, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+}
+
+void copyself(int fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}} expected-note{{va_list 'va' is copied onto itself}}
+  va_end(va);
+} // no-warning
+
+void copyselfUninit(int fst, ...) {
+  va_list va;
+  va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}} expected-note{{va_list 'va' is copied onto itself}}
+} // no-warning
+
+void copyOverwrite(int fst, ...) {
+  va_list va, va2;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  va_copy(va, va2); // expected-warning{{Initialized va_list 'va' is overwritten by an uninitialized one}} expected-note{{Initialized va_list 'va' is overwritten by an uninitialized one}}
+} // no-warning
+
+void copyUnint(int fst, ...) {
+  va_list va, va2;
+  va_copy(va, va2); // expected-warning{{Uninitialized va_list is copied}} expected-note{{Uninitialized va_list is copied}}
+}
+
+void g1(int fst, ...) {
+  va_list va;
+  va_end(va); // expected-warning{{va_end() is called on an uninitialized va_list}} expected-note{{va_end() is called on an uninitialized va_list}}
+}
+
+void g2(int fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  va_end(va); // expected-note{{Ended va_list}}
+  va_end(va); // expected-warning{{va_end() is called on an uninitialized va_list}} expected-note{{va_end() is called on an uninitialized va_list}}
+}
+
+void is_sink(int fst, ...) {
+  va_list va;
+  va_end(va); // expected-warning{{va_end() is called on an uninitialized va_list}} expected-note{{va_end() is called on an uninitialized va_list}}
+  *((volatile int *)0) = 1; //no-warning
+}
+
+// NOTE: this is invalid, as the man page of va_end requires that "Each invocation of va_start()
+// must be matched by a corresponding  invocation of va_end() in the same function."
+void ends_arg(va_list arg) {
+  va_end(arg);
+} //no-warning
+
+void uses_arg(va_list arg) {
+  (void)va_arg(arg, int);
+} //no-warning
+
+// This is the same function as the previous one, but it is called in call_uses_arg2(),
+// and the warning is generated during the analysis of call_uses_arg2().
+void inlined_uses_arg(va_list arg) {
+  (void)va_arg(arg, int); //expected-warning{{va_arg() is called on an uninitialized va_list}} expected-note{{va_arg() is called on an uninitialized va_list}}
+}
+
+void call_inlined_uses_arg(int fst, ...) {
+  va_list va;
+  inlined_uses_arg(va); // expected-note{{Calling 'inlined_uses_arg'}}
+}
+
+void call_vprintf_ok(int isstring, ...) {
+  va_list va;
+  va_start(va, isstring);
+  vprintf(isstring ? "%s" : "%d", va);
+  va_end(va);
+} //no-warning
+
+void call_vprintf_bad(int isstring, ...) {
+  va_list va;
+  vprintf(isstring ? "%s" : "%d", va); //expected-warning{{Function 'vprintf' is called with an uninitialized va_list argument}} expected-note{{Function 'vprintf' is called with an uninitialized va_list argument}} expected-note{{Assuming 'isstring' is 0}} expected-note{{'?' condition is false}}
+}
+
+void call_vsprintf_bad(char *buffer, ...) {
+  va_list va;
+  va_start(va, buffer); // expected-note{{Initialized va_list}}
+  va_end(va); // expected-note{{Ended va_list}}
+  vsprintf(buffer, "%s %d %d %lf %03d", va); //expected-warning{{Function 'vsprintf' is called with an uninitialized va_list argument}} expected-note{{Function 'vsprintf' is called with an uninitialized va_list argument}}
+}
+
+void call_some_other_func(int n, ...) {
+  va_list va;
+  some_library_function(n, va);
+} //no-warning
+

cfe/trunk/test/Analysis/valist-unterminated.c

+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.valist.Unterminated,alpha.valist.CopyToSelf -analyzer-output=text -analyzer-store=region -verify %s
+
+#include "Inputs/system-header-simulator-for-valist.h"
+
+void f1(int fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  return; // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}
+}
+
+void f2(int fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  va_end(va); // expected-note{{Ended va_list}}
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}}
+
+void f3(int fst, ...) {
+  va_list va, va2;
+  va_start(va, fst);
+  va_copy(va2, va); // expected-note{{Initialized va_list}}
+  va_end(va); // expected-warning{{Initialized va_list 'va2' is leaked}} expected-note{{Initialized va_list 'va2' is leaked}}
+}
+
+void f4(va_list *fst, ...) {
+  va_start(*fst, fst); // expected-note{{Initialized va_list}}
+  return; // expected-warning{{Initialized va_list is leaked}} expected-note{{Initialized va_list is leaked}}
+}
+
+void f5(va_list fst, ...) {
+  va_start(fst, fst);
+  //FIXME: this should cause a warning
+} // no-warning
+
+void f6(va_list *fst, ...) {
+  va_start(*fst, fst); // expected-note{{Initialized va_list}}
+  (void)va_arg(*fst, int);
+  //FIXME: this should NOT cause a warning
+  va_end(*fst); // expected-warning{{Initialized va_list is leaked}} expected-note{{Initialized va_list is leaked}}
+}
+
+void f7(int *fst, ...) {
+  va_list x;
+  va_list *y = &x;
+  va_start(*y,fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'x' is leaked}} expected-note{{Initialized va_list 'x' is leaked}}
+
+void f8(int *fst, ...) {
+  va_list x;
+  va_list *y = &x;
+  va_start(*y,fst);
+  va_end(x);
+} // no-warning 
+
+void reinit(int *fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}} expected-note{{Initialized va_list}}
+  va_start(va, fst); // expected-warning{{Initialized va_list 'va' is initialized again}} expected-note{{Initialized va_list 'va' is initialized again}}
+} // expected-warning{{Initialized va_list 'va' is leaked}} expected-note{{Initialized va_list 'va' is leaked}}
+
+void reinitOk(int *fst, ...) {
+  va_list va;
+  va_start(va, fst);
+  va_end(va);
+  va_start(va, fst);
+  va_end(va);
+} // no-warning
+
+void copyself(int fst, ...) {
+  va_list va;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}} expected-note{{va_list 'va' is copied onto itself}}
+  va_end(va);
+} // no-warning
+
+void copyselfUninit(int fst, ...) {
+  va_list va;
+  va_copy(va, va); // expected-warning{{va_list 'va' is copied onto itself}} expected-note{{va_list 'va' is copied onto itself}} 
+} // no-warning
+
+void copyOverwrite(int fst, ...) {
+  va_list va, va2;
+  va_start(va, fst); // expected-note{{Initialized va_list}}
+  va_copy(va, va2); // expected-warning{{Initialized va_list 'va' is overwritten by an uninitialized one}} expected-note{{Initialized va_list 'va' is overwritten by an uninitialized one}}
+} // no-warning
+
+//This only generates a warning for the valist.Uninitialized checker
+void copyUnint(int fst, ...) {
+  va_list va, va2;
+  va_copy(va, va2);
+} // no-warning
+
+void recopy(int fst, ...) {
+  va_list va, va2;
+  va_start(va, fst);
+  va_copy(va2, va); // expected-note{{Initialized va_list}}
+  va_copy(va2, va); // expected-warning{{Initialized va_list 'va2' is initialized again}} expected-note{{Initialized va_list 'va2' is initialized again}}
+  va_end(va);
+  va_end(va2);
+} //no-warning
+
+void doublemsg(int fst, ...) {
+  va_list va, va2;
+  va_start(va, fst), va_start(va2, fst); // expected-warning{{Initialized va_list 'va' is leaked}} expected-warning{{Initialized va_list 'va2' is leaked}} expected-note{{Initialized va_list}} expected-note{{Initialized va_list}} expected-note{{Initialized va_list}} expected-note{{Initialized va_list 'va' is leaked}} 
+} 
+
+void in_array(int fst, ...) {
+  va_list va_array[8];
+  va_start(va_array[3], fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'va_array[3]' is leaked}} expected-note{{Initialized va_list 'va_array[3]' is leaked}}
+
+struct containing_a_valist {
+  va_list vafield;
+  int foobar;
+};
+
+void in_struct(int fst, ...) {
+  struct containing_a_valist s;
+  va_start(s.vafield, fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 's.vafield' is leaked}} expected-note{{Initialized va_list 's.vafield' is leaked}}
+
+void casting(int fst, ...) {
+  char mem[sizeof(va_list)];
+  va_start(*(va_list *) mem, fst); // expected-note{{Initialized va_list}}
+} // expected-warning{{Initialized va_list 'mem[0]' is leaked}} expected-note{{Initialized va_list 'mem[0]' is leaked}}
+
+
+void castingOk(int fst, ...) {
+  char mem[sizeof(va_list)];
+  va_start(*(va_list *) mem, fst);
+  va_end(*(va_list *) mem);
+} // no-warning
+