[analyzer] PlacementNewChecker, properly handle array overhead (cookie)

[martong]

Summary

Currently, we warn if the given place is not enough to hold an extra
data for the array allocation overhead (cookie). I.e. if the size of the
place is equal to the size of the target. Also, we warn even if the size
of the place is greater than the size of the target. The current logic
does not know the size of the overhead(cookie), thus we just simply
warn. We warn even if the place is big enough to hold the overhead
and this is clearly wrong as it leads to false reports. This patch
eliminates the false reports and handles C++20 with special care.

Fixes #56264

C++17 and earlier standards state that:

new(2, f) T[5] results in a call of operator new[](sizeof(T) * 5 +
y, 2, f). Here, ... and y are non-negative unspecified values
representing array allocation overhead; the result of the
new-expression will be offset by this amount from the value returned
by operator new[]. This overhead may be applied in all array
new-expressions, including those referencing the library function
operator new[](std::size_t, void*) and other placement allocation
functions. The amount of overhead may vary from one invocation of
new to another.

Since the array overhead is an *unspecified* value, it makes sense to
warn only if the size of the Place is equal to the size of the Target.
Otherwise, e.g if we assumed that the overhead is sizeof(size_t),
then we might report a false positive.

Checking for array cookie does not makes sense if the standard is
C++20 or later. C++20 states that array overhead(cookie) is not
created if the new expression calls the non-allocating (placement)
form of the allocation function.
C++20, section 7.6.2.7 [expr.new], paragraph 15:

That argument shall be no less than the size of the object being
created; it may be greater than the size of the object being created
only if the object is an array and the allocation function is not a
non-allocating form (17.6.2.3).

C++20, section 7.6.2.7 [expr.new], paragraph 19:

This overhead may be applied in all array new-expressions, including
those referencing a placement allocation function, except when
referencing the library function operator new[](std::size_t, void*).

Related Defect Report:

2382. Array allocation overhead for non-allocating placement new

https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2382

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

157–159

"might" is not very convincing, it may cause a reaction like "I've no idea what it's talking about and the compiler itself isn't sure, must be false positive". Can we do anything to point the user in the right direction? Say, if this is implementation-defined, are we looking at a portability issue?

[martong]

• Replace "might" with "unspecified value"

[martong]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

157–159

Okay, I see your point. Let's dissect the corresponding sections of the standard:

new(2, f) T[5] results in a call of operator new[](sizeof(T) * 5 + y, 2, f).

Here, ... and y are non-negative unspecified values representing array allocation overhead;

The array overhead is an unspecified value. What does it mean exactly? My understanding is that this means it is implementation defined and the implementation is not required to document or guarantee anything. I came to this conclusion based on this stackoverflow question.

My interpretation could be wrong, but that does not matter. I think, we should just simply display the user what the standard says, and let them digest themselves the meaning of "unspecified". I am updating the patch like so.

[martong]

Ping

[isuckatcs]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

157–159

I also looked into this overhead question. In this stackoverflow answer someone links the same defect report that you did, and claims that since it's a defect report, it has been applied retroactively.

Apparently cppreference says the same. If you check the defect reports section on the bottom of the page you can see:

The following behavior-changing defect reports were applied retroactively to previously published C++ standards.

...
DR | Applied to | Behavior as published | Correct behavior
CWG 2382 | C++98 | non-allocating placement array new could require allocation overhead | such allocation overhead disallowed

So in my understanding you don't need to worry about this overhead at all. I hope this helps.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

157–159

Ok it sounds like we shouldn't emit any warnings at all here, unless we're somehow able to get a signal that the user is compiling their code with a pre-2019 compiler that hasn't been updated to reflect this defect report. One way we could get that signal is by putting the checker into the portability package which is off-by-default. Then we can explain the situation in the warning message:

"Storage provided to placement new is only {0} bytes, which is sufficient to hold the array data but placement new may require additional unspecified array allocation overhead on compilers that don't implement CWG 2382"

Then if the users enable portability but aren't interested in this warning, at least they know how to enable and disable checkers, so they can disable this specific check.

[steakhal]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

157–159

Personally, I think we should definitely not branch on the standard version.
This was never a thing anyway, AFAIK.

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
   SVal SizeOfPlace = getExtentSizeOfPlace(NE, C);
   const auto SizeOfTargetCI = SizeOfTarget.getAs<nonloc::ConcreteInt>();
   if (!SizeOfTargetCI)
     return true;
   const auto SizeOfPlaceCI = SizeOfPlace.getAs<nonloc::ConcreteInt>();
   if (!SizeOfPlaceCI)
     return true;
 
-  if ((SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue()) ||
-      (IsArrayTypeAllocated &&
-       SizeOfPlaceCI->getValue() >= SizeOfTargetCI->getValue())) {
+  const llvm::APSInt &PlaceSize = SizeOfPlaceCI->getValue();
+  const llvm::APSInt &TargetSize = SizeOfTargetCI->getValue();
+
+  bool ArrayOverheadCondition =
+      // C++17 and earlier standards state that:
+      //   new(2, f) T[5] results in a call of operator new[](sizeof(T) * 5 +
+      //   y, 2, f). Here, ... and y are non-negative unspecified values
+      //   representing array allocation overhead; the result of the
+      //   new-expression will be offset by this amount from the value returned
+      //   by operator new[]. This overhead may be applied in all array
+      //   new-expressions, including those referencing the library function
+      //   operator new[](std::size_t, void*) and other placement allocation
+      //   functions. The amount of overhead may vary from one invocation of
+      //   new to another.
+      // Since the array overhead is an *unspecified* value, it makes sense to
+      // warn only if the size of the Place is equal to the size of the Target.
+      // Otherwise, e.g if we assumed that the overhead is sizeof(size_t),
+      // then we might report a false positive.
+      IsArrayTypeAllocated && (PlaceSize == TargetSize) &&
+      // Checking for array cookie does not makes sense if the standard is
+      // C++20 or later. C++20 states that array overhead(cookie) is not
+      // created if the new expression calls the non-allocating (placement)
+      // form of the allocation function.
+      // C++20, section 7.6.2.7 [expr.new], paragraph 15:
+      //   That argument shall be no less than the size of the object being
+      //   created; it may be greater than the size of the object being created
+      //   only if the object is an array and the allocation function is not a
+      //   non-allocating form (17.6.2.3).
+      // C++20, section 7.6.2.7 [expr.new], paragraph 19:
+      //    This overhead may be applied in all array new-expressions, including
+      //    those referencing a placement allocation function, except when
+      //    referencing the library function operator new[](std::size_t, void*).
+      // Related Defect Report:
+      // 2382. Array allocation overhead for non-allocating placement new
+      // https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2382
+      !C.getASTContext().getLangOpts().CPlusPlus20;
+
+  if ((PlaceSize < TargetSize) || ArrayOverheadCondition) {
     if (ExplodedNode *N = C.generateErrorNode(C.getState())) {
       std::string Msg;
-      // TODO: use clang constant
-      if (IsArrayTypeAllocated &&
-          SizeOfPlaceCI->getValue() > SizeOfTargetCI->getValue())
-        Msg = std::string(llvm::formatv(
-            "{0} bytes is possibly not enough for array allocation which "
-            "requires {1} bytes. Current overhead requires the size of {2} "
-            "bytes",
-            SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue(),
-            SizeOfPlaceCI->getValue() - SizeOfTargetCI->getValue()));
-      else if (IsArrayTypeAllocated &&
-               SizeOfPlaceCI->getValue() == SizeOfTargetCI->getValue())
+      if (ArrayOverheadCondition)
         Msg = std::string(llvm::formatv(
             "Storage provided to placement new is only {0} bytes, "
-            "whereas the allocated array type requires more space for "
-            "internal needs",
-            SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+            "whereas the allocated array type requires a "
+            "non-negative unspecified amount for array allocation overhead",
+            PlaceSize, TargetSize));
       else
         Msg = std::string(llvm::formatv(
             "Storage provided to placement new is only {0} bytes, "
             "whereas the allocated type requires {1} bytes",
-            SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+            PlaceSize, TargetSize));
 
       auto R = std::make_unique<PathSensitiveBugReport>(SBT, Msg, N);
       bugreporter::trackExpressionValue(N, NE->getPlacementArg(0), *R);
       C.emitReport(std::move(R));
 
       return false;
     }
   }

clang/test/Analysis/placement-new.cpp

 // RUN: %clang_analyze_cc1 -std=c++11 %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=cplusplus.NewDelete \
 // RUN:   -analyzer-checker=cplusplus.PlacementNew \
-// RUN:   -analyzer-output=text -verify \
+// RUN:   -analyzer-output=text -verify=expected,cpp11 \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
+// RUN: %clang_analyze_cc1 -std=c++20 %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus.NewDelete \
+// RUN:   -analyzer-checker=cplusplus.PlacementNew \
+// RUN:   -analyzer-output=text -verify=expected \
 // RUN:   -triple x86_64-unknown-linux-gnu
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 void f() {
   short s;                    // expected-note {{'s' declared without an initial value}}
   long *lp = ::new (&s) long; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 3 {{}}
   (void)lp;

 void f() {
   Base b;                           // expected-note {{'b' initialized here}}
   Derived *dp = ::new (&b) Derived; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)dp;
 }
 } // namespace testHierarchy
 
 namespace testArrayTypesAllocation {
-void f1() {
+void test_less() {
   struct S {
     short a;
   };
-
-  // bad (not enough space).
   const unsigned N = 32;
-  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
-  ::new (buffer1) S[N];                            // expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+  alignas(S) unsigned char buffer1[sizeof(S) * N/2]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];                              // expected-warning{{Storage provided to placement new is only 32 bytes, whereas the allocated type requires 64 bytes}} expected-note 1 {{}}
 }
 
-void f2() {
+void test_equal() {
   struct S {
     short a;
   };
+  // Bad (not enough space).
+  // This should not appear if the standard is >= C++20.
+  // C++20 states that array overhead(cookie) is not created if the new
+  // expression calls the non-allocating (placement) form of the allocation
+  // function.
+  // C++20, section 7.6.2.7 [expr.new], paragraph 15:
+  //   That argument shall be no less than the size of the object being
+  //   created; it may be greater than the size of the object being created
+  //   only if the object is an array and the allocation function is not a
+  //   non-allocating form (17.6.2.3).
+  // C++20, section 7.6.2.7 [expr.new], paragraph 19:
+  //    This overhead may be applied in all array new-expressions, including
+  //    those referencing a placement allocation function, except when
+  //    referencing the library function operator new[](std::size_t, void*).
+  // Related Defect Report:
+  // 2382. Array allocation overhead for non-allocating placement new
+  // https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#2382
 
-  // maybe ok but we need to warn.
   const unsigned N = 32;
-  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
-  ::new (buffer2) S[N];                                          // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // cpp11-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];                            // cpp11-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires a non-negative unspecified amount for array allocation overhead}} cpp11-note 1 {{}}
+}
+
+int test_greater() {
+  struct s {
+    int x;
+  };
+  s arr[4];
+  new (arr + 1) s[1]; // no-warning
+  return 0;
 }
+
 } // namespace testArrayTypesAllocation
 
 namespace testStructAlign {
 void f1() {
   struct X {
     char a[9];
   } Xi; // expected-note {{'Xi' initialized here}}