[analyzer] Fix FP for cplusplus.placement new #149240 #150161
Conversation
… is strictly less than target size. Add test cases for when place size == or > or < than target size.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: None (Aethezz) ChangesFix false positive where warnings were asserted for placement new even when no additional space is requested The PlacementNewChecker incorrectly triggered warnings when the storage provided matched or exceeded the allocated type size, causing false positives. Now the warning triggers only when the provided storage is strictly less than the required size. Add test cases covering exact size, undersize, and oversize scenarios to validate the fix. Fixes #149240 Full diff: https://github.com/llvm/llvm-project/pull/150161.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
index 839c8bcd90210..4c4ca15efa4c9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp
@@ -111,32 +111,14 @@ bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
if (!SizeOfPlaceCI)
return true;
- if ((SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue()) ||
- (IsArrayTypeAllocated &&
- SizeOfPlaceCI->getValue() >= SizeOfTargetCI->getValue())) {
+ if ((SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue())) {
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())
- 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()));
- 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()));
+ 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()));
auto R = std::make_unique<PathSensitiveBugReport>(SBT, Msg, N);
bugreporter::trackExpressionValue(N, NE->getPlacementArg(0), *R);
diff --git a/clang/test/SemaCXX/placement-new-bounds.cpp b/clang/test/SemaCXX/placement-new-bounds.cpp
new file mode 100644
index 0000000000000..49a7352922140
--- /dev/null
+++ b/clang/test/SemaCXX/placement-new-bounds.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang -fsyntax-only -Xclang -analyze -Xclang -analyzer-checker=cplusplus.PlacementNew -Xclang -verify -std=c++17 %s
+
+#include <new>
+
+void test_exact_size() {
+ void *buf = ::operator new(sizeof(int)*2);
+ int *placement_int = new (buf) int[2]; // no-warning
+ placement_int[0] = 42;
+ ::operator delete(buf);
+}
+
+void test_undersize() {
+ void *buf = ::operator new(sizeof(int)*1);
+ // Remember the exact text including the checker name, or use regex for robustness.
+ int *placement_int = new (buf) int[2]; // expected-warning {{Storage provided to placement new is only 4 bytes, whereas the allocated type requires 8 bytes [cplusplus.PlacementNew]}}
+ placement_int[0] = 42;
+ ::operator delete(buf);
+}
+
+void test_oversize() {
+ void *buf = ::operator new(sizeof(int)*4);
+ int *placement_int = new (buf) int[2]; // no-warning
+ placement_int[0] = 42;
+ ::operator delete(buf);
+}
\ No newline at end of file
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
NagyDonat
left a comment
NagyDonat
left a comment
There was a problem hiding this comment.
The code that you're removing is not an accidental bug, but an intentional (although perhaps overzealous) feature that tries to warn about the fact that placement new for an array type may allocate an unspecified amount of overhead (extra memory) for internal needs.
According to a quick search this was a significant issue especially in Visual Studio (where it could cause memory corruption), but very recent versions of the standard (C++20 and later) declare that placement new of arrays must not introduce an overhead: https://stackoverflow.com/a/75418614
To improve the usefulness of this checker, I weakly support this change, but I would also like to see a second opinion from @steakhal @Xazax-hun @haoNoQ or other contributors.
I never understood the reasons of having metadata for placement-new. Certainly on linux it was not the case, but I'm skeptical if it was on any other platforms such as Windows. (prove me wrong). But unless it's proved that such a platform exists under some configuration, I see no benefit of having this warning. And even then, we should at least make this diagnostic conditional to only have it for the platforms where it's actually a thing. |
I don't think that there is any real reason for it, but I guess that some compilers reused code from the code of non-placement new.
This (admittedly 16 years old) stackoverflow post claims that (at that time) Visual Studio added four bytes of overhead to an array allocated with placement new: |
Now I see. The link is wrong. It does not link to what you think, but there was a comment that suggests what you said. Here is a slightly modified variant of that code, on some different msvc and clang 19. |
|
Hello, this is my first contribution and I am unsure of the process. I have tried fixing the formatting issues and failing test case from other unit tests. However, after viewing your discussions, is this still a valid issue? |
There was a problem hiding this comment.
I added some technical remarks in inline comments (including two comments that I accidentally posted separately, before this review).
If those are handled, I'm leaning towards merging this PR, because it I agree that these warnings (where the array would fit into the buffer where it's initialized, but the overhead of the array new can could technically cause issues) may be unwanted and confusing.
@steakhal What do you think, which would be the better: merging this PR or preserving the counter-intuitive bug reports that warn about potential unusual behavior of array placement new?
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
@steakhal What do you think, which would be the better: merging this PR or preserving the counter-intuitive bug reports that warn about potential unusual behavior of array placement new?
The PR looks good. I don't think we would ever want the diagnostics we are about to drop in this PR.
… test file to explain the removal of checker warning the rare case of allocating extra memory, and removed std::string explicit conversion operator
|
Hmm. The test case presented in that https://stackoverflow.com/questions/15254/can-placement-new-for-arrays-be-used-in-a-portable-way/75418614#75418614 link does show MSVC misbehaving as late as VS 2019 16.5 (for some reason MSVC 16.6 is missing on godbolt). https://godbolt.org/z/E891E31da And indeed, knowing what to look for, https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance?view=msvc-170 shows
And they called it out in the VS2019 16.7 blog: https://devblogs.microsoft.com/cppblog/c20-features-in-visual-studio-2019-versions-16-7-and-16-8/
So yeah, it wasn't just a textual bug in the standard, MSVC actually implemented it wrong. Now, since this was a defect report (rather than a C++17 -> 20 change), this fix is not gated behind I also find one other similar citation in github over at https://github.com/ska-sa/spead2/blob/f711c6bd44d73ffdaed594aa163707d8d33ce5bd/src/recv_chunk_stream.cpp#L140-L144 is one other place mentioning this, and saying it's only for "polymorphic classes" and not "scalar types". As best I can determine, the distinguishing factor is having a non-trivial destructor (which would make sense, given what the count is used for in a non-placement array new). // no extra space
//omitted ~A();
~A() = default;
// 8 bytes of extra space
~A() {};
virtual ~A() = default;
virtual ~A() final = default;So I guess the question is whether clang analyzer should care about an MSVC defect fixed 5 years ago, applying only to out-of-support versions (the only Visual Studio 2019 16.x still in support is the 16.11 servicing baseline, which is fixed). And clang itself never had the bug. I suppose in a very broad view it's a "portability" issue; but the code is standards-conforming. One could gate it behind |
|
Just for sake of linkdumping, https://developercommunity.visualstudio.com/t/Non-allocating-placement-array-new-somet/10777485?sort=active shows another (later) regression when the type in question come from a template parameter. |
|
This is really scary. Thanks for the really deep dive in the subject. |
|
Yeah, I'm also inclined to go with this as-is and just accept what is definitely conforming code. I.e. "it's not clang's job to give warnings about MSVC bugs". Especially bugs in end-of-life versions where the supported branches have been fixed. I certainly don't have any particular personal desire to overcomplicate this, I just want the false-positive to stop showing up (my projects are using g++, clang, and MSVC2022). But clang does sometimes try pretty hard at being msvc-compatible , so I wasn't sure but what there might be other opinions... it certainly seems possible to make it conditional on |
steakhal
left a comment
steakhal
left a comment
There was a problem hiding this comment.
I looked at the content of the PR now, and I suggested a couple of places to improve.
Overall, I'm very happy with this change.
Thank you everyone for sticking around and putting the energy into thins one.
| llvm::formatv("Storage provided to placement new is only {0} bytes, " | ||
| "whereas the allocated type requires {1} bytes", | ||
| SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()); | ||
| // TODO: use clang constants |
There was a problem hiding this comment.
I'm curious what the original author had in mind for this comment. Do you have an idea what constants it might refer to?
It might refer to the "overhead amount" or "internal needs" which is in the code that we drop right now. This would suggest to me that this comment would become dangling after this change, thus we should just drop it along with the code. WDYT?
There was a problem hiding this comment.
Yeah, I think the TODO comment likely referred to replacing the 'overhead amount' or 'internal needs' literals with Clang constants. I will go ahead and drop that dangling comment.
|
@Aethezz Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
5 participants
Fix false positive where warnings were asserted for placement new even when no additional space is requested
The PlacementNewChecker incorrectly triggered warnings when the storage provided matched or exceeded the allocated type size, causing false positives. Now the warning triggers only when the provided storage is strictly less than the required size.
Add test cases covering exact size, undersize, and oversize scenarios to validate the fix.
Fixes #149240