[Analyzer][VLASizeChecker] Check VLA size in typedef and sizeof.

[balazske]

Summary

The check of VLA size was done previously for variable declarations
(of VLA type) only. Now it is done for typedef (and type-alias)
and sizeof expressions with VLA too.

[xazax.hun]

Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

576

At first, it might be confusing why do we need to handle typedefs at all. I think it might worth adding a comment about VLAs.

[Szelethus]

We check now whether the argument of typedef and sizeof is an incorrect VLA, but what other examples are we potentially forgetting? The warning message states that "Declared variable-length array (VLA) has negative size", but we are not actually looking for declarations, but try to guess which statements may be declarations. Did we cover all realistic cases?

In D79072#2010142, @xazax.hun wrote:

Overall the changes look good to me here. I had a small nit inline. But I wonder if we actually should add more code in the analyzer core to better model the sizes of memory regions corresponding to the VLAs.

I think this is that code :) I think the existing regions/values we have can explain VLAs well, don't you think?

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

75–79

This, or at least parts of this comment should be placed for the function declaration rather :) Also, you explain the "how", but not the "why". You remove the VLALast argument in the followup patch, why do we add it here?

190–191

Should this ever happen? We seem to assert in ExprEngine.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

576

Definitely. If there is a standard out there, it would be great to quote it. Also, it might be worth going on a bit of a tangent:

Although we're mostly looking for variable declarations here, the size of variable-length arrays are evaluated at the typedef, as specified by §x.x.x.x.

Also, whats up with using? VLAs can occur in C++ as well, can they not?

[balazske]

I do not know what other than typedef or sizeof (and variable declaration) can contain VLA. To my knowledge VLA is not normally supported in C++ and should not be used anyway (there are better ways for doing it) so some funny C++ things with VLA may not work. Anyway the patch is about "Check VLA size in typedef and sizeof", later other things can be added if required. The CERT rule ARR32-C (this is to be supported by the change) mentions only typedef and sizeof.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

75–79

It explains "how" because it is a comment inside the function. I can add comment to the function too. VLALast is used in this change, the next change contains additional refactoring that makes it unneeded, because the evaluation of size expressions (that uses VLA and VLALast) is moved into this function.

190–191

This was part of the old code, it may be wrong but not related to this change.

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

576

There is TypedefNameDecl used and not TypedefDecl, so this should work for using too. (But main intent is to work on C source code, as VLA is not C++ friendly.) I found the typedef in a C standard draft at 6.7.7 (that tells about VLA size). The size expression must be evaluated when the typedef is encountered.

[balazske]

Added better comments.

[martong]

I'd split this patch into two as well.

1. [NFC] Refactoring parts
2. The actual extra additions about sizeof and typedef.

WDYT?

Other than that looks good.

[balazske]

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

577

Should we not do something else with the VLA size expressions (not only call the checker callbacks) because they should be evaluated, are these handled in some other way automatically? (The CFG should contain these expressions already so these should be evaluated by the engine, and this place is only to make the checkers aware of typedef statements.)

[balazske]

This change is relatively small and the refactoring like part (introduction of checkVLA if I think correct?) is connected to its use. The other change is add of a new function (callback). This is probably small enough to go into one change and we can see why the new function checkVLA is needed. So my vote is to not split this change.

[Szelethus]

In D79072#2025120, @martong wrote:

I'd split this patch into two as well.

1. [NFC] Refactoring parts
2. The actual extra additions about sizeof and typedef.

WDYT?

Other than that looks good.

I wouldn't change this now, but for the future, changes like splitting functions into checkVLA and checkVLASize should deserve their own revision, because the actual new logic would only take a few lines, and a few minutes to review. The biggest pain point is that both this, and the followup patch change the same code and it makes it difficult to know whats the new functionality and what is just reorganization.

[martong]

In D79072#2026553, @balazske wrote:

This change is relatively small and the refactoring like part (introduction of checkVLA if I think correct?) is connected to its use. The other change is add of a new function (callback). This is probably small enough to go into one change and we can see why the new function checkVLA is needed. So my vote is to not split this change.

Okay, you convinced me. LGTM!

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

577

Yeah, I agree with @balazske. This could be used for anything else in the future, not just for VLA. It is more generic than that.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace clang;
 using namespace ento;
 using namespace taint;
 
 namespace {
-class VLASizeChecker : public Checker< check::PreStmt<DeclStmt> > {
+class VLASizeChecker
+    : public Checker<check::PreStmt<DeclStmt>,
+                     check::PreStmt<UnaryExprOrTypeTraitExpr>> {
   mutable std::unique_ptr<BugType> BT;
   enum VLASize_Kind { VLA_Garbage, VLA_Zero, VLA_Tainted, VLA_Negative };
 
+  /// Check a VLA for validity.
+  /// Every dimension of the array is checked for validity, and
+  /// dimension sizes are collected into 'VLASizes'. 'VLALast' is set to the
+  /// innermost VLA that was encountered.
+  /// In "int vla[x][2][y][3]" this will be the array for index "y" (with type
+  /// int[3]). 'VLASizes' contains 'x', '2', and 'y'. Returns null or a new
+  /// state where the size is validated for every dimension.
+  ProgramStateRef checkVLA(CheckerContext &C, ProgramStateRef State,
+                           const VariableArrayType *VLA,
+                           const VariableArrayType *&VLALast,
+                           llvm::SmallVector<const Expr *, 2> &VLASizes) const;
+
+  /// Check one VLA dimension for validity.
+  /// Returns null or a new state where the size is validated.
   ProgramStateRef checkVLASize(CheckerContext &C, ProgramStateRef State,
                                const Expr *SizeE) const;
 
   void reportBug(VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
                  CheckerContext &C,
                  std::unique_ptr<BugReporterVisitor> Visitor = nullptr) const;
 
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const;
+  void checkPreStmt(const UnaryExprOrTypeTraitExpr *UETTE,
+                    CheckerContext &C) const;
 };
 } // end anonymous namespace
 
+ProgramStateRef
+VLASizeChecker::checkVLA(CheckerContext &C, ProgramStateRef State,
+                         const VariableArrayType *VLA,
+                         const VariableArrayType *&VLALast,
+                         llvm::SmallVector<const Expr *, 2> &VLASizes) const {
+  assert(VLA && "Function should be called with non-null VLA argument.");
+
+  VLALast = nullptr;
+  // Walk over the VLAs for every dimension until a non-VLA is found.
+  // There is a VariableArrayType for every dimension (fixed or variable) until
+  // the most inner array that is variably modified.
+  while (VLA) {
+    const Expr *SizeE = VLA->getSizeExpr();
+    State = checkVLASize(C, State, SizeE);
+    if (!State)
+      return nullptr;
+    VLASizes.push_back(SizeE);
+    VLALast = VLA;
+    VLA = C.getASTContext().getAsVariableArrayType(VLA->getElementType());
+  };
+  assert(VLALast &&
+         "Array should have at least one variably-modified dimension.");
+
+  return State;
+}
+
 ProgramStateRef VLASizeChecker::checkVLASize(CheckerContext &C,
                                              ProgramStateRef State,
                                              const Expr *SizeE) const {
   SVal SizeV = C.getSVal(SizeE);
 
   if (SizeV.isUndef()) {
     reportBug(VLA_Garbage, SizeE, State, C);
     return nullptr;
void VLASizeChecker::reportBug(
   auto report = std::make_unique<PathSensitiveBugReport>(*BT, os.str(), N);
   report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());
   bugreporter::trackExpressionValue(N, SizeE, *report);
   C.emitReport(std::move(report));
 }
 
 void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
   if (!DS->isSingleDecl())
     return;
 
-  const VarDecl *VD = dyn_cast<VarDecl>(DS->getSingleDecl());
-  if (!VD)
-    return;
-
   ASTContext &Ctx = C.getASTContext();
   SValBuilder &SVB = C.getSValBuilder();
   ProgramStateRef State = C.getState();
+  QualType TypeToCheck;
 
-  const VariableArrayType *VLA = Ctx.getAsVariableArrayType(VD->getType());
+  const VarDecl *VD = dyn_cast<VarDecl>(DS->getSingleDecl());
+
+  if (VD)
+    TypeToCheck = VD->getType().getCanonicalType();
+  else if (const auto *TND = dyn_cast<TypedefNameDecl>(DS->getSingleDecl()))
+    TypeToCheck = TND->getUnderlyingType().getCanonicalType();
+  else
+    return;
+
+  const VariableArrayType *VLA = Ctx.getAsVariableArrayType(TypeToCheck);
   if (!VLA)
     return;
 
+  // Check the VLA sizes for validity.
   llvm::SmallVector<const Expr *, 2> VLASizes;
   const VariableArrayType *VLALast = nullptr;
-  // Walk over the VLAs for every dimension until a non-VLA is found.
-  // Collect the sizes in VLASizes, put the most inner VLA to `VLALast`.
-  // In "vla[x][2][y][3]" this will be the array for index "y".
-  // There is a VariableArrayType for every dimension (here "x", "2", "y")
-  // until a non-vla is found.
-  while (VLA) {
-    const Expr *SizeE = VLA->getSizeExpr();
-    State = checkVLASize(C, State, SizeE);
-    if (!State)
-      return;
-    VLASizes.push_back(SizeE);
-    VLALast = VLA;
-    VLA = Ctx.getAsVariableArrayType(VLA->getElementType());
-  };
-  assert(VLALast &&
-         "Array should have at least one variably-modified dimension.");
+
+  State = checkVLA(C, State, VLA, VLALast, VLASizes);
+
+  if (!VD)
+    return;
 
   // VLASizeChecker is responsible for defining the extent of the array being
   // declared. We do this by multiplying the array length by the element size,
   // then matching that with the array region's extent symbol.
 
   CanQualType SizeTy = Ctx.getSizeType();
   // Get the element size.
   CharUnits EleSize = Ctx.getTypeSizeInChars(VLALast->getElementType());
void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
 
   // Assume should not fail at this point.
   assert(State);
 
   // Remember our assumptions!
   C.addTransition(State);
 }
 
+void VLASizeChecker::checkPreStmt(const UnaryExprOrTypeTraitExpr *UETTE,
+                                  CheckerContext &C) const {
+  // Want to check for sizeof.
+  if (UETTE->getKind() != UETT_SizeOf)
+    return;
+
+  // Ensure a type argument.
+  if (!UETTE->isArgumentType())
+    return;
+
+  const VariableArrayType *VLA =
+      C.getASTContext().getAsVariableArrayType(UETTE->getTypeOfArgument());
+  // Ensure that the type is a VLA.
+  if (!VLA)
+    return;
+
+  ProgramStateRef State = C.getState();
+
+  llvm::SmallVector<const Expr *, 2> VLASizes;
+  const VariableArrayType *VLALast = nullptr;
+  State = checkVLA(C, State, VLA, VLALast, VLASizes);
+  if (!State)
+    return;
+
+  C.addTransition(State);
+}
+
 void ento::registerVLASizeChecker(CheckerManager &mgr) {
   mgr.registerChecker<VLASizeChecker>();
 }
 
 bool ento::shouldRegisterVLASizeChecker(const CheckerManager &mgr) {
   return true;
 }

clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp

if (CL->isGLValue())
       V = CLLoc;
   }
 
   B.generateNode(CL, Pred, State->BindExpr(CL, LCtx, V));
 }
 
 void ExprEngine::VisitDeclStmt(const DeclStmt *DS, ExplodedNode *Pred,
                                ExplodedNodeSet &Dst) {
+  if (isa<TypedefNameDecl>(*DS->decl_begin())) {
+    // C99 6.7.7 "Any array size expressions associated with variable length
+    // array declarators are evaluated each time the declaration of the typedef
+    // name is reached in the order of execution."
+    // The checkers should know about typedef to be able to handle VLA size
+    // expressions.
+    ExplodedNodeSet DstPre;
+    getCheckerManager().runCheckersForPreStmt(DstPre, Pred, DS, *this);
+    getCheckerManager().runCheckersForPostStmt(Dst, DstPre, DS, *this);
+    return;
+  }
+
   // Assumption: The CFG has one DeclStmt per Decl.
   const VarDecl *VD = dyn_cast_or_null<VarDecl>(*DS->decl_begin());
 
   if (!VD) {
     //TODO:AZ: remove explicit insertion after refactoring is done.
     Dst.insert(Pred);
     return;
   }

clang/test/Analysis/vla.c

static void check_negative_sized_VLA_11_sub(int x)
   int vla[x]; // no-warning
 }
 
 void check_negative_sized_VLA_11(int x) {
   if (x > 0)
     check_negative_sized_VLA_11_sub(x);
 }
 
+void check_VLA_typedef() {
+  int x = -1;
+  typedef int VLA[x]; // expected-warning{{Declared variable-length array (VLA) has negative size}}
+}
+
+size_t check_VLA_sizeof() {
+  int x = -1;
+  size_t s = sizeof(int[x]); // expected-warning{{Declared variable-length array (VLA) has negative size}}
+  return s;
+}
+
 // Multi-dimensional arrays.
 
 void check_zero_sized_VLA_multi1(int x) {
   if (x)
     return;
 
   int vla[10][x]; // expected-warning{{Declared variable-length array (VLA) has zero size}}
 }