[analyzer] Initialize regions returned by CXXNew to undefined

[Szelethus]

Summary

Discourse mail: https://discourse.llvm.org/t/analyzer-why-do-we-suck-at-modeling-c-dynamic-memory/65667

malloc() returns a piece of uninitialized dynamic memory. We were (almost) always able to model this behaviour. Its C++ counterpart, operator new is a lot more complex, because it allows for initialization, the most complicated of which is the usage of constructors.

We gradually became better in modeling constructors, but for some reason, most likely for reasons lost in history, we never actually modeled the case when the memory returned by operator new was just simply uninitialized. This patch (attempts) to fix this tiny little error.

[Szelethus]

Just a note on the test files -- I've diverged from the usual stance of just changing what the new output is, to modifying the test files. The reason is that reading an undefined value is a fatal error, leading to the analyzer to stop analyzing prematurely, and I think these cases were trying to test something else, not uninitialized value usage.

[steakhal]

Awesome!
Have you measured how often would this change introduce new garbage value warnings?
At the other side of the spectrum it could also hide reports, because it sinks the path too soon due to the falsely binding uninitialized value there.
WDYT?

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

957

Yeey, finally we will have this :D

I wonder if we could query from the ASTContext if we have a trivially constructible class typeor something as a first approximation.

clang/test/Analysis/NewDelete-checker-test.cpp

388–393

This change seems unrelated.
Could you elaborate on that?

clang/test/Analysis/new.cpp

179–180

You should probably adjust this comment.

[Szelethus]

Some early results:

https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=on&run=%2acxxnew_baseline&newcheck=%2acxxnew_patched&diff-type=New

I'm still waiting on a few projects, but this is something. A few reports from alpha.security.ArrayBound, I still need to look into that.

This report from core.uninitialized.Assign is interesting. First off, I'd appreciate if the "storing uninitialized value" was placed inside the notes about the call to QScopedArrayPointer's constructor. Otherwise, the report looks correct.

Not too happy about this report by core.CallAndMessage. Message 9 is placed on the line

applyColorTransform(d->colorSpace.transformationToColorSpace(colorSpace));

Messag 10 talks about what happens in applyColorTransform, but the important thing happens at the evaluation of the argument, which is not described, so this isn't a very good bug report, can't tell whether its false.

Reports from core.UndefinedBinaryOperatorResult here and alpha.core.Conversion here are interesting, because there doesn't need to be caused by uninitialized dynamic memory, but rather the fact that the memory is being modeled at all. I suspect this is due us maybe discarding array size information or something similar when the value held in UnknownVal?

The rest of the reports seem nobrainder gains.

Interstingly, 3 reports are lost from alpha.security.ArrayBound, but I'm not sure how seriously do we take this checker.

[martong]

In D135375#3849261, @Szelethus wrote:

Some early results:

https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved&is-unique=on&run=%2acxxnew_baseline&newcheck=%2acxxnew_patched&diff-type=New

I'm still waiting on a few projects, but this is something. A few reports from alpha.security.ArrayBound, I still need to look into that.

This report from core.uninitialized.Assign is interesting. First off, I'd appreciate if the "storing uninitialized value" was placed inside the notes about the call to QScopedArrayPointer's constructor. Otherwise, the report looks correct.

Not too happy about this report by core.CallAndMessage. Message 9 is placed on the line

applyColorTransform(d->colorSpace.transformationToColorSpace(colorSpace));

Messag 10 talks about what happens in applyColorTransform, but the important thing happens at the evaluation of the argument, which is not described, so this isn't a very good bug report, can't tell whether its false.

Yes, I agree, this bug-report is hard to follow. Seems like the colorSpace argument from step 1 is flowing through step 10. I wonder if colorSpace at step 1 is evaluated as Undefined? Could that happen? Did you have this report before your change?

Reports from core.UndefinedBinaryOperatorResult here

This seems to be justified, at step 11 we initialize the variable with a negative value. I think, this has nothing to do with your changes.

and alpha.core.Conversion here are interesting, because there doesn't need to be caused by uninitialized dynamic memory, but rather the fact that the memory is being modeled at all. I suspect this is due us maybe discarding array size information or something similar when the value held in UnknownVal?

The rest of the reports seem nobrainder gains.

Interstingly, 3 reports are lost from alpha.security.ArrayBound, but I'm not sure how seriously do we take this checker.

I think these new reports and the lost reports are the result of the changed analysis paths. With your patch, there are new bug reports which introduce sink nodes which were not there previously. Consequently, the set of analyzed paths and thus the new results can be different.

[martong]

I am happy with the change and with the result. However, this colorSpace related report is a bit concerning. Could you please check if colorSpace at step 1 is evaluated as Undefined? Could that happen? Or that is (should be) Unknown since that is a parameter of a top-level analyzed function? Did you have this report before your change? I think this would worth to be further analyzed, maybe just directing the analyzer to analyze only the QImage::convertedToColorSpace top-level function.

[NoQ]

Yay I'm glad that you got to implement that!!

I'd appreciate if the "storing uninitialized value" was placed inside the notes about the call to QScopedArrayPointer's constructor.

It should not be placed inside the notes about the call to constructor, because it doesn't happen during constructor invocation. It happens during operator new invocation, which is strictly before the constructor.

I guess we could improve the note to specify that it was operator new that stored the value; this could help in general case as well.

Separately, there *should* be notes inside the constructor as well, about the fact that the constructor *did not* initialize the fields, even though it *could have* (it literally had one job!) - You know, your favorite problem for at least two unrelated reasons =)

Edit, never mind, the constructor wasn't supposed to initialize that data at all. We're talking about the poinee of the smart pointer, not the smart pointer itself. The pointee is a raw int and doesn't have a constructor.

Messag 10 talks about what happens in applyColorTransform, but the important thing happens at the evaluation of the argument, which is not described, so this isn't a very good bug report, can't tell whether its false.

Yeah looks like trackExpressionValue() wasn't able to track it inside transformationToColorSpace().

[Szelethus]

Seems like the issues mentioned above are real, but orthogonal to this patch. Would it be okay to address them in followup patches? @martong @NoQ

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

957

And a result skip the rest of this function?

clang/test/Analysis/NewDelete-checker-test.cpp

388–393

The test case in its original version would have emitted an uninitialized use report, which is fine, but the intention is to test double deletes, not uninitialized use, hence the seemingly unrelated change.

[martong]

In D135375#3882491, @Szelethus wrote:

Seems like the issues mentioned above are real, but orthogonal to this patch. Would it be okay to address them in followup patches? @martong @NoQ

I am okay with it. Thanks!

[donat.nagy]

Commit rGde2547329b41ad6ea4ea876d12731bde5a6b64c5 accidentally refers to this review, but in fact it belongs to D148355.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp

 //===- ExprEngineCXX.cpp - ExprEngine support for C++ -----------*- 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 file defines the C++ expression evaluation engine.
 //
 //===----------------------------------------------------------------------===//
 
-#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
-#include "clang/Analysis/ConstructionContext.h"
 #include "clang/AST/DeclCXX.h"
-#include "clang/AST/StmtCXX.h"
 #include "clang/AST/ParentMap.h"
+#include "clang/AST/StmtCXX.h"
+#include "clang/Analysis/ConstructionContext.h"
 #include "clang/Basic/PrettyStackTrace.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 
 using namespace clang;
 using namespace ento;
 
 void ExprEngine::CreateCXXTemporaryObject(const MaterializeTemporaryExpr *ME,
                                           ExplodedNode *Pred,
                                           ExplodedNodeSet &Dst) {
   StmtNodeBuilder Bldr(Pred, Dst, *currBldrCtx);
for (ExplodedNode *I : DstPostCall) {
     // this is not correct because the operator new's prototype always says that
     // it returns a 'void *'. So we should change the type of the symbol,
     // and then evaluate the cast over the symbolic pointer from 'void *' to
     // the object pointer type. But without changing the symbol's type it
     // is breaking too much to evaluate the no-op symbolic cast over it, so we
     // skip it for now.
     ProgramStateRef State = I->getState();
     SVal RetVal = State->getSVal(CNE, LCtx);
+    // [basic.stc.dynamic.allocation] (on the return value of an allocation
+    // function):
+    // "The order, contiguity, and initial value of storage allocated by
+    // successive calls to an allocation function are unspecified."
+    State = State->bindDefaultInitial(RetVal, UndefinedVal{}, LCtx);
 
     // If this allocation function is not declared as non-throwing, failures
     // /must/ be signalled by exceptions, and thus the return value will never
     // be NULL. -fno-exceptions does not influence this semantics.
     // FIXME: GCC has a -fcheck-new option, which forces it to consider the case
     // where new can return NULL. If we end up supporting that option, we can
     // consider adding a check for it here.
     // C++11 [basic.stc.dynamic.allocation]p3.

clang/test/Analysis/NewDelete-checker-test.cpp

namespace reference_count {
   }
 }
 
 // Test double delete
 class DerefClass{
 public:
   int *x;
   DerefClass() {}
-  ~DerefClass() {*x = 1;}
+  ~DerefClass() {
+    int i = 0;
+    x = &i;
+    *x = 1;
+  }
 };
 
 void testDoubleDeleteClassInstance() {
   DerefClass *foo = new DerefClass();
   delete foo;
   delete foo; // newdelete-warning {{Attempt to delete released memory}}
 }
 
 class EmptyClass{

clang/test/Analysis/cxx-member-initializer-const-field.cpp

 // RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
 
 // This tests false-positive issues related to PR48534.
 //
 // Essentially, having a default member initializer for a constant member does
 // not necessarily imply the member will have the given default value.
 
 struct WithConstructor {
   int *const ptr = nullptr;
   WithConstructor(int *x) : ptr(x) {}
 
   static auto compliant() {
-    WithConstructor c(new int);
+    WithConstructor c(new int{});
     return *(c.ptr); // no warning
   }
 
   static auto compliantWithParam(WithConstructor c) {
     return *(c.ptr); // no warning
   }
 
   static auto issue() {
     WithConstructor c(nullptr);
     return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
   }
 };
 
 struct RegularAggregate {
   int *const ptr = nullptr;
 
   static int compliant() {
-    RegularAggregate c{new int};
+    RegularAggregate c{new int{}};
     return *(c.ptr); // no warning
   }
 
   static int issue() {
     RegularAggregate c;
     return *(c.ptr); // expected-warning{{Dereference of null pointer (loaded from field 'ptr')}}
   }
 };

clang/test/Analysis/new-ctor-conservative.cpp

 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify -analyzer-config eagerly-assume=false %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
 };
 
 void checkConstructorInlining() {
   S *s = new S;
   clang_analyzer_eval(s->x == 1); // expected-warning{{TRUE}}
 }
 
-void checkNewPOD() {
+void checkNewPODunit() {
   int *i = new int;
-  clang_analyzer_eval(*i == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(*i == 0); // expected-warning{{The left operand of '==' is a garbage value [core.UndefinedBinaryOperatorResult]}}
+}
+
+void checkNewPOD() {
   int *j = new int();
   clang_analyzer_eval(*j == 0); // expected-warning{{TRUE}}
   int *k = new int(5);
   clang_analyzer_eval(*k == 5); // expected-warning{{TRUE}}
 }
 
 void checkNewArray() {
   S *s = new S[10];

clang/test/Analysis/new-ctor-recursive.cpp

 void *operator new(size_t size, S *place) {
   if (!place)
     return new S();
   return place;
 }
 
 void testThatCharConstructorIndeedYieldsGarbage() {
   S *s = new S(ConstructionKind::Garbage);
-  clang_analyzer_eval(s->x == 0); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(s->x == 1); // expected-warning{{UNKNOWN}}
-  // FIXME: This should warn, but MallocChecker doesn't default-bind regions
-  // returned by standard operator new to garbage.
-  s->x += 1; // no-warning
+  clang_analyzer_eval(s->x == 0); // expected-warning{{The left operand of '==' is a garbage value [core.UndefinedBinaryOperatorResult]}}
+  clang_analyzer_eval(s->x == 1);
+  s->x += 1;
   delete s;
 }
 
 
 void testChainedOperatorNew() {
   S *s;
   // * Evaluate standard new.
   // * Evaluate constructor S(3).

clang/test/Analysis/new.cpp

void testAggregateNew() {
   struct Point { int x, y; };
   new Point{1, 2}; // no crash
 
   Point p;
   new (&p) Point{1, 2}; // no crash
   clang_analyzer_eval(p.x == 1); // expected-warning{{TRUE}}
   clang_analyzer_eval(p.y == 2); // expected-warning{{TRUE}}
 }
 
-//--------------------------------
-// Incorrectly-modelled behavior
-//--------------------------------
-
 int testNoInitialization() {
   int *n = new int;
 
-  // Should warn that *n is uninitialized.
-  if (*n) { // no-warning
+  if (*n) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
     delete n;
     return 0;
   }
   delete n;
   return 1;
 }
 
+//===----------------------------------------------------------------------===//
+// Incorrectly-modelled behavior.
+//===----------------------------------------------------------------------===//
+
 int testNoInitializationPlacement() {
   int n;
   new (&n) int;
 
   if (n) { // expected-warning{{Branch condition evaluates to a garbage value}}
     return 0;
   }
   return 1;

clang/test/Analysis/placement-new.cpp

void f() {
   long *lp = ::new (buf + 4) long; // expected-warning{{Storage provided to placement new is only -4 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)lp;
 }
 } // namespace testNegativeSize
 
 namespace testHeapAllocatedBuffer {
 void g2() {
   char *buf = new char[2];     // expected-note {{'buf' initialized here}}
+                               // FIXME: The message is misleading -- we should
+                               // state that a pointer to an uninitialized value
+                               // is stored.
+                               // expected-note@-4{{Storing uninitialized value}}
   long *lp = ::new (buf) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)lp;
 }
 } // namespace testHeapAllocatedBuffer
 
 namespace testMultiDimensionalArray {
 void f() {
   char buf[2][3];              // expected-note {{'buf' initialized here}}

clang/test/Analysis/reinterpret-cast.cpp

namespace PR15345 {
   public:
     void (*f)();
     int x;
   };
 
   class Derived : public Base {};
 
   void test() {
-  Derived* p;
-  *(reinterpret_cast<void**>(&p)) = new C;
-  p->f();
-
-    // We should still be able to do some reasoning about bindings.
-    p->x = 42;
-    clang_analyzer_eval(p->x == 42); // expected-warning{{TRUE}}
+    Derived* p;
+    *(reinterpret_cast<void**>(&p)) = new C;
+    p->f(); // expected-warning{{Called function pointer is an uninitialized pointer value [core.CallAndMessage]}}
   };
-}
+} // namespace PR15345
 
 int trackpointer_std_addressof() {
   int x;
   int *p = (int*)&reinterpret_cast<const volatile char&>(x);
   *p = 6;
   return x; // no warning
 }
 

clang/test/Analysis/uninit-const.cpp

 void doStuff6(const int& c);
 void doStuff4(const int y);
 void doStuff3(int& g);
 void doStuff_uninit(const int *u);
 
 
 int f10(void) {
   int *ptr;
-
-  ptr = new int; //
-  if(*ptr) {
+                 // FIXME: The message is misleading -- we should state that
+                 // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+  if(*ptr) { // expected-warning{{Branch condition evaluates to a garbage value [core.uninitialized.Branch]}}
+             // expected-note@-1 {{Branch condition evaluates to a garbage value}}
     doStuff4(*ptr);
   }
   delete ptr;
   return 0;
 }
 
 int f9(void) {
   int *ptr;
-
-  ptr = new int; //
-
-  doStuff_uninit(ptr); // no warning
+                 // FIXME: The message is misleading -- we should state that
+                 // a pointer to an uninitialized value is stored.
+  ptr = new int; // expected-note{{Storing uninitialized value}}
+                 // expected-note@-1{{Value assigned to 'ptr'}}
+  doStuff_uninit(ptr); // expected-warning{{1st function call argument is a pointer to uninitialized value [core.CallAndMessage]}}
+                       // expected-note@-1{{1st function call argument is a pointer to uninitialized value}}
   delete ptr;
   return 0;
 }
 
 int f8(void) {
   int *ptr;
 
   ptr = new int;