[analyzer] Warn if the size of the array in `new[]` is undefined

[isuckatcs]

Summary

@martong I kept your idea in mind for this warning message and implemented it as a new checker if you don't mind it.

If the size of an array is Undefined when it's allocated, this patch emits a warning, and ends the analysis.

[NoQ]

Oh thanks a lot for handling this case!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1527

Expressions cannot be of reference type. They use value kinds instead (lvalue, xvalue, etc). So I suspect your tests pass without this check.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1508

A slightly more natural way to do this in path-sensitive analysis is to invoke

const auto *ACE = dyn_cast<CXXAllocatorCall>(&Call);

and then use that subclass's accessor methods to access values.

It doesn't look like this subclass actually provides an accessor for the size value though. We should add one!

Maybe also add a getter that asks whether the call is for new[] vs. new, and then assert that it's new[] in the size value getter.

2293–2296

I think making it a separate checker is slightly better. It's more natural to group it together with the uninitialized variable checks than with memory leak / use-after-free checks.

Like, generally, I wish we had a more flexible system of "hashtags" that could allow us to group checkers in different ways, probably overlapping (so this check could be #NewDelete #UninitializedVariable). But that's a story for another time :D

2313

I believe you can simply add a trackExpressionValue() instead. It'll display where the value comes from through path notes.

[isuckatcs]

• Addressed comments
• Separated the checker from MallocChecker to a new one

[isuckatcs]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2313

I've added it but I also left this message. If someone only runs clang++ --analyze source.cpp then I think it feels better to receive a bit more descriptive warning message.

[isuckatcs]

Fixed a typo

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

29

I think we no longer need this mutable lazy initialized BugType hack. See FuchsiaHandleChecker.cpp for an example. While we no longer need this hack, we have not cleaned up the existing checks yet :(

41

I think we could add a convenience function to CXXAllocatorCall that would return an SVal for the size expression.

[xazax.hun]

clang/test/Analysis/undefined-new-element.cpp

3

Is there a comment missing here?

[isuckatcs]

Addressed comments

[isuckatcs]

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

29

I'm not really familiar with checker developement. Why did we need this hack?

clang/test/Analysis/undefined-new-element.cpp

3

This checker is in alpha.core at the moment. Should I just put it directly to core instead?

[xazax.hun]

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

29

Long story short, we need lazy initialization because some information is only available when a bug is reported (and not when the check is initialized). This is due to the architecture of CSA. In the old times the lazy initialization was the responsibility of the checks and later we moved this hack to the engine itself to make writing checks easier and cleaner.

clang/test/Analysis/undefined-new-element.cpp

3

I might be missing something, but I found it surprising that the RUN line in line 2 is not commented out while the RUN line in line 3 is.

[isuckatcs]

Addressed comments

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

29

I see, thanks for the explanation.

clang/test/Analysis/undefined-new-element.cpp

3

Oh, that is a typo I didn't notice. I moved the whole command into one line instead, it's short enough for that I think.

[xazax.hun]

I am not sure whether we actually use exclamation marks in diagnostic texts. Otherwise this looks good to me. Maybe adding expected-note's to the test would be nice, but overall this looks good to me.

[NoQ]

Great!

I think this checker can be turned on by default pretty quickly. We should run it over some code but other than that it seems to be in good shape from the start.

I agree with Gabor, we don't usually have exclamation marks in warnings. It could definitely be exciting to have exclamation marks! But I'm worried that since clang is part of multiple finished GUI products, various UI/UX review boards won't be happy with us for that :( So we limit exclamation marks to assertion failures. Side note, clang warning style and grammar doesn't seem to be documented anywhere. Side note, static analyzer warning style and grammar is completely different from clang warning style and grammar (we expect our warnings to be complete sentences).

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

308

Maybe alpha.core.uninitialized then, with the intent to eventually put it into core.uninitialized?

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1038–1040

This function doesn't seem to ever return None (?)

1043–1045

I think it's cleaner to assert(isArray()) here and avoid the optional. The caller would do something like

if (!Call->isArray())
  return;

SVal V = Call->getArraySizeVal();

and it communicates the intent behind the code much nicer.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

2313

If someone only runs clang++ --analyze source.cpp then I think it feels better to receive a bit more descriptive warning message.

Most static analyzer warnings are barely ever understandable without path notes, your extra text won't help much most of the time. We basically don't support running the static analyzer without reading path notes, so I think we should ignore this scenario. Especially given that this facility doesn't cover all the cases so it may make diagnostics look worse.

It's definitely weird that such basic invocation is unsupported. I think we should flip the default output to --analyzer-output text, but it may break backwards compatibility in weird ways because the current default output is actually plist (so the command you quoted will produce a weird .plist file in the current directory as an output) so IDEs can run this command and consume this machine-readable output.

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

32

I recommend checkPreCall. This will cause the new checker to run *before* MallocChecker. It's also generally natural to check preconditions in PreCall and evaluate postconditions in PostCall.

[martong]

@martong I kept your idea in mind for this warning message and implemented it as a new checker if you don't mind it. If the size of an array is Undefined when it's allocated, this patch emits a warning, and ends the analysis.

Thank you for taking care of this!

clang/test/Analysis/undefined-new-element.cpp

43–46

I think it would make sense to have a test for an array placement new expression as well.
Something like:

int n;
void* buffer = malloc(sizeof(std::string) * 10);
std::string* p = ::new (buffer) std::string[n]; // expected-warning{{Undefined element count!}}

[isuckatcs]

Addressed comments

[isuckatcs]

Updated

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

1038–1040

getArraySize() returns an Optional<const clang::Expr *>, so it can return None.

[isuckatcs]

Updated

[NoQ]

Ok I have some warning text bikeshedding but generally the patch looks great and the checker should probably be enabled by default from the start, or otherwise very soon.

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

29

Let's point out that we're talking about new[]:

"Undefined array element count in new[]"

50

It's probably easier to do this inside the function.

62

Static analyzer warnings are traditionally full sentences. Let's make this more complete:

"Element count in new[] is a garbage value"

(it also avoids the term "undefined", which we typically avoid and we should have probably avoided in UndefinedArraySubscriptChecker as well; it's not precise enough, may be confusing)

[isuckatcs]

Addressed comments

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

50

I prefer to keep the logics separated, so it's more obvious which part of the code is doing what.

[isuckatcs]

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

1752

This doesn't fix any known bugs, I've just noticed that it's error prone and fixed it in this patch.

[steakhal]

Looks great!

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

449

This line suggests that this checker is documented at the checkers.rst, however, I cannot see the corresponding entry.

clang/test/Analysis/undefined-new-element.cpp

9

IDK, it's probably personal, but I dislike newlines here. And it also feels weird that in the llvm coding style we have the curly openings at the line endings instead of at the next line.

[isuckatcs]

Added a documentation into checkers.rst.

clang/test/Analysis/undefined-new-element.cpp

9

I like the newlines there. Also I reformatted this file with the llvm style.

[isuckatcs]

Fixed docs build error

[isuckatcs]

Fixed the broken test file. Clang-format put a line break between {{ and }} and that caused the failure.

[NoQ]

This comment has been deleted.

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

80

You can use mgr.getLangOpts().CPlusPlus to avoid registering the checker on non-C++ translation units.

clang/test/Analysis/undefined-new-element.cpp

2

Let's make sure tracking works by adding -analyzer-output=text.

[isuckatcs]

• Addressed comments
• Moved the checker to core from alpha.core

[isuckatcs]

Updated

[NoQ]

Looks great. I agree that it can go straight to core because our undefined value checkers are roughly all the same and we're already pretty good at writing them. The checker appears to be very quiet in my experience, I'm yet to see a warning in the wild, but I think the path note tests give enough signal that warnings are going to look good.

I have one tiny nitpick that should be addressed before committing.

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

63

Static analyzer warnings start with a capital letter! The idea is that they're complete sentences, unlike compiler warnings.

[isuckatcs]

Addressed the nit

[isuckatcs]

Updated the documentation

clang/docs/analyzer/checkers.rst

 
 .. code-block:: c
 
  void test(int x) {
    var = 77 / x;
    if (x == 0) { } // warn
  }
 
+.. _alpha-core-uninitialized-NewArraySize:
+
+alpha.core.uninitialized.NewArraySize
+"""""""""""""""""""""""""""""""""""""
+
+Check if the element count in new[] is garbage or undefined.
+
+.. code-block:: cpp
+
+  void test() {
+    int n;
+    int *arr = new int[n]; // warn: element count in new[] is a garbage value
+    delete[] arr;
+  }
+
 alpha.cplusplus
 ^^^^^^^^^^^^^^^
 
 .. _alpha-cplusplus-DeleteWithNonVirtualDtor:
 
 alpha.cplusplus.DeleteWithNonVirtualDtor (C++)
 """"""""""""""""""""""""""""""""""""""""""""""
 Reports destructions of polymorphic objects with a non-virtual destructor in their base class.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def PthreadLockBase : Checker<"PthreadLockBase">,
   Documentation<NotDocumented>,
   Hidden;
 
 def C11LockChecker : Checker<"C11Lock">,
   HelpText<"Simple lock -> unlock checker">,
   Dependencies<[PthreadLockBase]>,
   Documentation<HasDocumentation>;
 
 } // end "alpha.core"
 
 //===----------------------------------------------------------------------===//
 // Nullability checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Nullability in {
 
 def NullabilityBase : Checker<"NullabilityBase">,

 def UndefCapturedBlockVarChecker : Checker<"CapturedBlockVariable">,
   HelpText<"Check for blocks that capture uninitialized values">,
   Documentation<NotDocumented>;
 
 def ReturnUndefChecker : Checker<"UndefReturn">,
   HelpText<"Check for uninitialized values being returned to the caller">,
   Documentation<HasDocumentation>;
 
+def UndefinedNewArraySizeChecker : Checker<"NewArraySize">,
+  HelpText<"Check if the size of the array in a new[] expression is undefined">,
+  Documentation<HasDocumentation>;
+
 } // end "core.uninitialized"
 
 //===----------------------------------------------------------------------===//
 // Unix API checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = CString in {
 
 def CStringModeling : Checker<"CStringModeling">,
   HelpText<"The base of several CString related checkers. On it's own it emits "
            "no reports, but adds valuable information to the analysis when "
            "enabled.">,
   Documentation<NotDocumented>,
   Hidden;

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h

public:
   unsigned getNumImplicitArgs() const {
     return getOriginExpr()->passAlignment() ? 2 : 1;
   }
 
   unsigned getNumArgs() const override {
     return getOriginExpr()->getNumPlacementArgs() + getNumImplicitArgs();
   }
 
+  bool isArray() const { return getOriginExpr()->isArray(); }
+
+  Optional<const clang::Expr *> getArraySizeExpr() const {
+    return getOriginExpr()->getArraySize();
+  }
+
+  SVal getArraySizeVal() const {
+    assert(isArray() && "The allocator call doesn't allocate and array!");
+
+    return getState()->getSVal(*getArraySizeExpr(), getLocationContext());
+  }
+
   const Expr *getArgExpr(unsigned Index) const override {
     // The first argument of an allocator call is the size of the allocation.
     if (Index < getNumImplicitArgs())
       return nullptr;
     return getOriginExpr()->getPlacementArg(Index - getNumImplicitArgs());
   }
 
   /// Number of placement arguments to the operator new() call. For example,

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   TraversalChecker.cpp
   TrustNonnullChecker.cpp
   TrustReturnsNonnullChecker.cpp
   UndefBranchChecker.cpp
   UndefCapturedBlockVarChecker.cpp
   UndefResultChecker.cpp
   UndefinedArraySubscriptChecker.cpp
   UndefinedAssignmentChecker.cpp
+  UndefinedNewArraySizeChecker.cpp
   UninitializedObject/UninitializedObjectChecker.cpp
   UninitializedObject/UninitializedPointee.cpp
   UnixAPIChecker.cpp
   UnreachableCodeChecker.cpp
   VforkChecker.cpp
   VLASizeChecker.cpp
   ValistChecker.cpp
   VirtualCallChecker.cpp

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

if (isStandardNewDelete(Call)) {
     return;
   }
 
   checkOwnershipAttr(Call, C);
 }
 
 // Performs a 0-sized allocations check.
 ProgramStateRef MallocChecker::ProcessZeroAllocCheck(
     const CallEvent &Call, const unsigned IndexOfSizeArg, ProgramStateRef State,
     Optional<SVal> RetVal) {
   if (!State)
     return nullptr;
 
   if (!RetVal)
     RetVal = Call.getReturnValue();
 
   const Expr *Arg = nullptr;
 
   if (const CallExpr *CE = dyn_cast<CallExpr>(Call.getOriginExpr())) {
     Arg = CE->getArg(IndexOfSizeArg);
   } else if (const CXXNewExpr *NE =
                  dyn_cast<CXXNewExpr>(Call.getOriginExpr())) {
     if (NE->isArray()) {
       Arg = *NE->getArraySize();
     } else {
       return State;
     }
   } else
     llvm_unreachable("not a CallExpr or CXXNewExpr");
 
   assert(Arg);
 
   auto DefArgVal =
       State->getSVal(Arg, Call.getLocationContext()).getAs<DefinedSVal>();
 
   if (!DefArgVal)
ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
   const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
   DefinedSVal RetVal = svalBuilder.getConjuredHeapSymbolVal(CE, LCtx, Count)
       .castAs<DefinedSVal>();
   State = State->BindExpr(CE, C.getLocationContext(), RetVal);
 
   // Fill the region with the initialization value.
   State = State->bindDefaultInitial(RetVal, Init, LCtx);
 
+  // If Size is somehow undefined at this point, this line prevents a crash.
+  if (Size.isUndef())
+    Size = UnknownVal();
+
   // Set the region's extent.
   State = setDynamicExtent(State, RetVal.getAsRegion(),
                            Size.castAs<DefinedOrUnknownSVal>(), svalBuilder);
 
   return MallocUpdateRefState(C, CE, State, Family);
 }
 
 static ProgramStateRef MallocUpdateRefState(CheckerContext &C, const Expr *E,
if (ExplodedNode *N = C.generateErrorNode()) {
     R->addRange(Range);
     C.emitReport(std::move(R));
   }
 }
 
 void MallocChecker::HandleMismatchedDealloc(CheckerContext &C,
                                             SourceRange Range,
                                             const Expr *DeallocExpr,
                                             const RefState *RS, SymbolRef Sym,
                                             bool OwnershipTransferred) const {
 
   if (!ChecksEnabled[CK_MismatchedDeallocatorChecker]) {
     C.addSink();
     return;
   }
 
   if (ExplodedNode *N = C.generateErrorNode()) {
     if (!BT_MismatchedDealloc)
       BT_MismatchedDealloc.reset(
           new BugType(CheckNames[CK_MismatchedDeallocatorChecker],
                       "Bad deallocator", categories::MemoryError));
 
     SmallString<100> buf;
     llvm::raw_svector_ostream os(buf);
 
     const Expr *AllocExpr = cast<Expr>(RS->getStmt());
     SmallString<20> AllocBuf;
     llvm::raw_svector_ostream AllocOs(AllocBuf);
     SmallString<20> DeallocBuf;
     llvm::raw_svector_ostream DeallocOs(DeallocBuf);
 
     if (OwnershipTransferred) {
       if (printMemFnName(DeallocOs, C, DeallocExpr))
         os << DeallocOs.str() << " cannot";
       else
         os << "Cannot";
 

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

+//===--- UndefinedNewArraySizeChecker.cpp -----------------------*- C++ -*--==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines UndefinedNewArraySizeChecker, a builtin check in ExprEngine
+// that checks if the size of the array in a new[] expression is undefined.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.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;
+
+namespace {
+class UndefinedNewArraySizeChecker : public Checker<check::PreCall> {
+
+private:
+  BugType BT{this, "Undefined array element count in new[]",
+             categories::LogicError};
+
+public:
+  void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
+  void HandleUndefinedArrayElementCount(CheckerContext &C, SVal ArgVal,
+                                        const Expr *Init,
+                                        SourceRange Range) const;
+};
+} // namespace
+
+void UndefinedNewArraySizeChecker::checkPreCall(const CallEvent &Call,
+                                                CheckerContext &C) const {
+  if (const auto *AC = dyn_cast<CXXAllocatorCall>(&Call)) {
+    if (!AC->isArray())
+      return;
+
+    auto *SizeEx = *AC->getArraySizeExpr();
+    auto SizeVal = AC->getArraySizeVal();
+
+    if (SizeVal.isUndef())
+      HandleUndefinedArrayElementCount(C, SizeVal, SizeEx,
+                                       SizeEx->getSourceRange());
+  }
+}
+
+void UndefinedNewArraySizeChecker::HandleUndefinedArrayElementCount(
+    CheckerContext &C, SVal ArgVal, const Expr *Init, SourceRange Range) const {
+
+  if (ExplodedNode *N = C.generateErrorNode()) {
+
+    SmallString<100> buf;
+    llvm::raw_svector_ostream os(buf);
+
+    os << "element count in new[] is a garbage value";
+
+    auto R = std::make_unique<PathSensitiveBugReport>(BT, os.str(), N);
+    R->markInteresting(ArgVal);
+    R->addRange(Range);
+    bugreporter::trackExpressionValue(N, Init, *R);
+
+    C.emitReport(std::move(R));
+  }
+}
+
+void ento::registerUndefinedNewArraySizeChecker(CheckerManager &mgr) {
+  mgr.registerChecker<UndefinedNewArraySizeChecker>();
+}
+
+bool ento::shouldRegisterUndefinedNewArraySizeChecker(
+    const CheckerManager &mgr) {
+  return mgr.getLangOpts().CPlusPlus;
+}

clang/test/Analysis/Issue56873.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
 
-void clang_analyzer_warnIfReached();
-
 struct S {
 };
 
 void Issue56873_1() {
     int n;
 
     // This line used to crash
-    S *arr = new S[n];
-    
-    clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+    S *arr = new S[n]; // expected-warning{{element count in new[] is a garbage value}}
 }
 
 void Issue56873_2() {
     int n;
 
     // This line used to crash
-    int *arr = new int[n];
-    
-    clang_analyzer_warnIfReached();  // expected-warning{{REACHABLE}}
+    int *arr = new int[n]; // expected-warning{{element count in new[] is a garbage value}}
 }

clang/test/Analysis/undefined-new-element.cpp

+// RUN: %clang_analyze_cc1 %s -analyzer-checker=core.uninitialized.NewArraySize -analyzer-output=text -verify
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void checkUndefinedElmenetCountValue() {
+  int n;
+  // expected-note@-1{{'n' declared without an initial value}}
+
+  int *arr = new int[n]; // expected-warning{{element count in new[] is a garbage value}}
+  // expected-note@-1{{element count in new[] is a garbage value}}
+}
+
+void checkUndefinedElmenetCountMultiDimensionalValue() {
+  int n;
+  // expected-note@-1{{'n' declared without an initial value}}
+
+  auto *arr = new int[n][5]; // expected-warning{{element count in new[] is a garbage value}}
+  // expected-note@-1{{element count in new[] is a garbage value}}
+}
+
+void checkUndefinedElmenetCountReference() {
+  int n;
+  // expected-note@-1{{'n' declared without an initial value}}
+
+  int &ref = n;
+  // expected-note@-1{{'ref' initialized here}}
+
+  int *arr = new int[ref]; // expected-warning{{element count in new[] is a garbage value}}
+  // expected-note@-1{{element count in new[] is a garbage value}}
+}
+
+void checkUndefinedElmenetCountMultiDimensionalReference() {
+  int n;
+  // expected-note@-1{{'n' declared without an initial value}}
+
+  int &ref = n;
+  // expected-note@-1{{'ref' initialized here}}
+
+  auto *arr = new int[ref][5]; // expected-warning{{element count in new[] is a garbage value}}
+  // expected-note@-1{{element count in new[] is a garbage value}}
+}
+
+int foo() {
+  int n;
+
+  return n;
+}
+
+void checkUndefinedElmenetCountFunction() {
+  int *arr = new int[foo()]; // expected-warning{{element count in new[] is a garbage value}}
+  // expected-note@-1{{element count in new[] is a garbage value}}
+}
+
+void checkUndefinedElmenetCountMultiDimensionalFunction() {
+  auto *arr = new int[foo()][5]; // expected-warning{{element count in new[] is a garbage value}}
+  // expected-note@-1{{element count in new[] is a garbage value}}
+}
+
+void *malloc(size_t);
+
+void checkUndefinedPlacementelementCount() {
+  int n;
+  // expected-note@-1{{'n' declared without an initial value}}
+  
+  void *buffer = malloc(sizeof(std::string) * 10);
+  std::string *p =
+      ::new (buffer) std::string[n]; // expected-warning{{element count in new[] is a garbage value}}
+  // expected-note@-1{{element count in new[] is a garbage value}}
+}