Draft:[analyzer]:fix valistChecker false negative in windows platform

[mzyKi]

#72618 fixed false negative in valist-uninitialized-no-undef.c in windows platform;

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Exile (mzyKi)

Changes

fixed #72618 inlined_uses_arg call_vprintf_bad, but still fail in call_vsprintf_bad in valist-uninitialized-no-undef.c

---

Full diff: https://github.com/llvm/llvm-project/pull/72951.diff

1 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp (+34-2)

diff --git a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
index 2d1b873abf73f09..0ac7b092aa86278 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -27,8 +27,9 @@ 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> {
+class ValistChecker
+    : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
+                     check::PreStmt<DeclStmt>, check::DeadSymbols> {
   mutable std::unique_ptr<BugType> BT_leakedvalist, BT_uninitaccess;
 
   struct VAListAccepter {
@@ -49,11 +50,13 @@ class ValistChecker : public Checker<check::PreCall, check::PreStmt<VAArgExpr>,
   bool ChecksEnabled[CK_NumCheckKinds] = {false};
   CheckerNameRef CheckNames[CK_NumCheckKinds];
 
+  void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const;
   void checkPreStmt(const VAArgExpr *VAA, CheckerContext &C) const;
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
 
 private:
+  bool isWinValistType(const VarDecl *VD) const;
   const MemRegion *getVAListAsRegion(SVal SV, const Expr *VAExpr,
                                      bool &IsSymbolic, CheckerContext &C) const;
   const ExplodedNode *getStartCallSite(const ExplodedNode *N,
@@ -160,6 +163,35 @@ void ValistChecker::checkPreCall(const CallEvent &Call,
   }
 }
 
+bool ValistChecker::isWinValistType(const VarDecl *VD) const {
+  ASTContext &Ctx = VD->getASTContext();
+  QualType T = VD->getType();
+  if (T.isNull()) {
+    return false;
+  }
+  return T.getDesugaredType(Ctx)->isPointerType() &&
+         T.getDesugaredType(Ctx)->getPointeeType()->isCharType();
+}
+
+void ValistChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
+  for (const auto *I : DS->decls()) {
+    if (const auto *D = dyn_cast<VarDecl>(I)) {
+      if (isWinValistType(D)) {
+        ProgramStateRef State = C.getState();
+        const LocationContext *LC = C.getLocationContext();
+        const VarRegion *R = State->getRegion(D, LC);
+        MemRegionManager &MR = R->getMemRegionManager();
+        SValBuilder &SVB = C.getSValBuilder();
+        const ElementRegion *ER =
+            MR.getElementRegion(C.getASTContext().CharTy,
+                                SVB.makeZeroArrayIndex(), R, C.getASTContext());
+        State = State->bindLoc(State->getLValue(D, LC), SVB.makeLoc(ER), LC);
+        C.addTransition(State);
+      }
+    }
+  }
+}
+
 const MemRegion *ValistChecker::getVAListAsRegion(SVal SV, const Expr *E,
                                                   bool &IsSymbolic,
                                                   CheckerContext &C) const {

[NagyDonat]

While this commit may fix some of the false negatives, it is introducing a very general and (if I understood it correctly) semantically incorrect modeling step, so I don't think that it can be merged.

The first issue is that isWinValistType returns true for all variables declared as char * (including those that have nothing to do with va_list). If you want to limit the effects of this callback to va_list objects (which is probably necessary -- it would be bad if this checker e.g. influenced the modeling of string handling), you need to check the name of the typedef which is used to refer to the type.

The second issue is that that the call

State = State->bindLoc(State->getLValue(D, LC), SVB.makeLoc(ER), LC);

is makes the analyzer act as if the declaration va_list va; was followed by va = (char *)&va; (and analogously, the analyzer will "imagine" that all declarations like char *p; are followed by p = (char *)&p;). I understand that this is not what you wanted to write, but the memory model / type system of the analyzer is very counter-intuitive, so I think this is what will actually happen:

• When you call const VarRegion *R = State->getRegion(D, LC);, it returns the region [1] which corresponds to the declaration D, so in the case of va_list va; this is the 64-bit uninitialized stack region that stores the va_list aka char * pointer value.
• When you call ElementRegion *ER = MR.getElementRegion(C.getASTContext().CharTy, SVB.makeZeroArrayIndex(), R, C.getASTContext()), you turn the VarRegion *R (which is a region storing a va_list aka char * value) into a region that has the same starting address (because the index/offset is 0), but stores a char value. This is basically equivalent to performing a char ** to char * cast.
• ProgramState::bindLoc(Loc LV, SVal V, ...) returns an updated state where the the lvalue/location LV contains/holds the value V (which is an arbitrary SVal [2]: it could be either an integer or a pointer value). In our case the location LV is local variable va, while V is a symbolic location, which in this context represents a pointer value.

[1] VarRegion and other subtypes of MemRegion represents a region/range in the memory which has a starting address (that may be concrete or symbolic), may have an known extent (that may be concrete or symbolic) and may have a known type.
[2] A SVal is a potentially symbolic value that may be Undefined (value from an uninitalized variable or result of other illegal operation), Unknown (presumably contains a "regular" value, but the analyzer doesn't know it), a Loc (an lvalue / location, essentially a pointer value) or a NonLoc (an integer or boolean).

[NagyDonat]

In fact, as I think about this, I realized that it's probably a bad idea to perform "initialization" for each va_list variable in a check::PreStmt<DeclStmt> callback -- e.g. because in theory the programmer could write code like

void f(int fst, ...) {
  va_list *pva = (va_list *)malloc(sizeof(va_list));
  va_start(*pva, fst);
  /* use and release the va_list */
  free(pva);
}

and here we don't have any DeclStmt that declares a va_list. (Of course I don't think that this would occur in real code, but it's still better to give a robust and general solution.)

[NagyDonat]

..and it turns out that ValistChecker already has a function called ValistChecker::getVAListAsRegion that handles different representations of va_list under various systems. (I was not familiar with it because it was added by @Xazax-hun after the initial commit https://reviews.llvm.org/D15227 that contains my contributions.)

I'm strongly suspect that extending this function is the "right way" to adapt this checker to the windows environment that you're studying.

[mzyKi]

..and it turns out that ValistChecker already has a function called ValistChecker::getVAListAsRegion that handles different representations of va_list under various systems. (I was not familiar with it because it was added by @Xazax-hun after the initial commit https://reviews.llvm.org/D15227 that contains my contributions.)

I'm strongly suspect that extending this function is the "right way" to adapt this checker to the windows environment that you're studying.

Thanks for your review.I worte isWinValistType incorrectly.And I will try to find another solution.