[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

28

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

40

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

2

Is there a comment missing here?

[isuckatcs]

Addressed comments

[isuckatcs]

clang/lib/StaticAnalyzer/Checkers/UndefinedNewArraySizeChecker.cpp

28

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

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

2

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

28

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

2

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

28

I see, thanks for the explanation.

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

2

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

313

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

450

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/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>;
 
+def UndefinedNewArraySizeChecker : Checker<"NewArraySize">,
+  HelpText<"Check if the size of the array in a new[] expression is undefined">,
+  Documentation<HasDocumentation>;
+
+
 } // end "alpha.core"
 
 //===----------------------------------------------------------------------===//
 // Nullability checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = Nullability in {
 
 def NullabilityBase : Checker<"NullabilityBase">,
def ReturnUndefChecker : Checker<"UndefReturn">,
   HelpText<"Check for uninitialized values being returned to the caller">,
   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();
+  }
+
   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,
   /// standard std::nothrow operator new and standard placement new both have
   /// 1 implicit argument (size) and 1 placement argument, while regular
   /// operator new() has 1 implicit argument and 0 placement arguments.
   const Expr *getPlacementArgExpr(unsigned Index) const {

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::PostCall> {
+
+private:
+  mutable std::unique_ptr<BugType> BT;
+
+public:
+  void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+  void HandleUndefinedArrayElementCount(CheckerContext &C, SVal ArgVal,
+                                        const Expr *Init,
+                                        SourceRange Range) const;
+};
+} // namespace
+
+void UndefinedNewArraySizeChecker::checkPostCall(const CallEvent &Call,
+                                                 CheckerContext &C) const {
+  if (const auto *AC = dyn_cast<CXXAllocatorCall>(&Call)) {
+    if (!AC->isArray())
+      return;
+
+    auto *SizeEx = *AC->getArraySizeExpr();
+
+    const auto State = Call.getState();
+    const auto *LCtx = Call.getLocationContext();
+
+    auto SizeVal = State->getSVal(SizeEx, LCtx);
+
+    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()) {
+    if (!BT)
+      BT.reset(new BuiltinBug(this, "Undefined array element count"));
+
+    SmallString<100> buf;
+    llvm::raw_svector_ostream os(buf);
+
+    os << "Undefined element count! ";
+
+    // Try to be more informative...
+    os << "The value ";
+    Init = Init->IgnoreCasts();
+
+    if (const auto *DRE = dyn_cast<DeclRefExpr>(Init))
+      os << "stored in '" << *DRE->getDecl() << "' ";
+    else if (const auto *CE = dyn_cast<CallExpr>(Init)) {
+      os << "returned ";
+
+      if (auto CalleeDecl = dyn_cast_or_null<NamedDecl>(CE->getCalleeDecl()))
+        os << "from '" << *CalleeDecl << "' ";
+      else
+        os << "'[*here*]' ";
+    } else
+      os << "'[*here*]' ";
+
+    os << "is ";
+    ArgVal.dumpToStream(os);
+    os << "!";
+
+    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 true;
+}

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

+// RUN: %clang_analyze_cc1 %s \
+RUN:   -analyzer-checker=alpha.core \
+// RUN:   -verify
+
+void checkUndefinedElmenetCountValue()
+{
+    int n;
+
+    int *arr = new int[n]; // expected-warning{{Undefined element count!}}
+}
+
+void checkUndefinedElmenetCountMultiDimensionalValue()
+{
+    int n;
+
+    auto *arr = new int[n][5]; // expected-warning{{Undefined element count!}}
+}
+
+void checkUndefinedElmenetCountReference()
+{
+    int n;
+
+    int& ref = n;
+
+    int *arr = new int[ref]; // expected-warning{{Undefined element count!}}
+}
+
+void checkUndefinedElmenetCountMultiDimensionalReference()
+{
+    int n;
+
+    int& ref = n;
+
+    auto *arr = new int[ref][5]; // expected-warning{{Undefined element count!}}
+}
+
+int foo()
+{
+    int n;
+
+    return n;
+}
+
+void checkUndefinedElmenetCountFunction()
+{
+    int *arr = new int[foo()]; // expected-warning{{Undefined element count!}}
+}
+
+void checkUndefinedElmenetCountMultiDimensionalFunction()
+{
+    auto *arr = new int[foo()][5]; // expected-warning{{Undefined element count!}}
+}