[UBSan] Fix incorrect alignment reported when global new returns an o… #152532
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen Author: None (gbMattN) Changes…ffset pointer This problem and fix was originally raised on Phabricator, https://reviews.llvm.org/D116861, by belkiss. I wasn't able to find them on github or the discourse so I'm re-raising this. The problem was reported as follows > I found that ubsan will report an incorrect alignment for a type in case it is allocated with the global operator new (without alignment), if we have it return an offset ptr. Full diff: https://github.com/llvm/llvm-project/pull/152532.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index c7e53331fe05f..ecc71a6343fd6 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -1798,7 +1798,7 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
SkippedChecks.set(SanitizerKind::Null, nullCheck);
EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall,
E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
- result, allocType, result.getAlignment(), SkippedChecks,
+ result, allocType, allocAlign, SkippedChecks,
numElements);
EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
diff --git a/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp b/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp
new file mode 100644
index 0000000000000..4586a0932affd
--- /dev/null
+++ b/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp
@@ -0,0 +1,32 @@
+// RUN: %clangxx -fsanitize=alignment %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not="runtime error" -allow-empty
+// Disable with msan and tsan because they also override global new
+// UNSUPPORTED: ubsan-msan, ubsan-tsan
+
+#include <cassert>
+#include <cstddef>
+#include <cstdlib>
+
+void *operator new(std::size_t count) {
+ constexpr const size_t offset = 8;
+
+ // allocate a bit more so we can safely offset it
+ void *ptr = std::malloc(count + offset);
+
+ // verify malloc returned 16 bytes aligned mem
+ static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ == 16,
+ "Global new doesn't return 16 bytes aligned memory!");
+ assert((reinterpret_cast<std::ptrdiff_t>(ptr) &
+ (__STDCPP_DEFAULT_NEW_ALIGNMENT__ - 1)) == 0);
+
+ return static_cast<char *>(ptr) + offset;
+}
+
+struct Param {
+ void *_cookie1;
+ void *_cookie2;
+};
+
+static_assert(alignof(Param) == 8, "Param struct alignment must be 8 bytes!");
+
+int main() { Param *p = new Param; }
|
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (gbMattN) Changes…ffset pointer This problem and fix was originally raised on Phabricator, https://reviews.llvm.org/D116861, by belkiss. I wasn't able to find them on github or the discourse so I'm re-raising this. The problem was reported as follows > I found that ubsan will report an incorrect alignment for a type in case it is allocated with the global operator new (without alignment), if we have it return an offset ptr. Full diff: https://github.com/llvm/llvm-project/pull/152532.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index c7e53331fe05f..ecc71a6343fd6 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -1798,7 +1798,7 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
SkippedChecks.set(SanitizerKind::Null, nullCheck);
EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall,
E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
- result, allocType, result.getAlignment(), SkippedChecks,
+ result, allocType, allocAlign, SkippedChecks,
numElements);
EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
diff --git a/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp b/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp
new file mode 100644
index 0000000000000..4586a0932affd
--- /dev/null
+++ b/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp
@@ -0,0 +1,32 @@
+// RUN: %clangxx -fsanitize=alignment %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not="runtime error" -allow-empty
+// Disable with msan and tsan because they also override global new
+// UNSUPPORTED: ubsan-msan, ubsan-tsan
+
+#include <cassert>
+#include <cstddef>
+#include <cstdlib>
+
+void *operator new(std::size_t count) {
+ constexpr const size_t offset = 8;
+
+ // allocate a bit more so we can safely offset it
+ void *ptr = std::malloc(count + offset);
+
+ // verify malloc returned 16 bytes aligned mem
+ static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ == 16,
+ "Global new doesn't return 16 bytes aligned memory!");
+ assert((reinterpret_cast<std::ptrdiff_t>(ptr) &
+ (__STDCPP_DEFAULT_NEW_ALIGNMENT__ - 1)) == 0);
+
+ return static_cast<char *>(ptr) + offset;
+}
+
+struct Param {
+ void *_cookie1;
+ void *_cookie2;
+};
+
+static_assert(alignof(Param) == 8, "Param struct alignment must be 8 bytes!");
+
+int main() { Param *p = new Param; }
|
|
@llvm/pr-subscribers-clang Author: None (gbMattN) Changes…ffset pointer This problem and fix was originally raised on Phabricator, https://reviews.llvm.org/D116861, by belkiss. I wasn't able to find them on github or the discourse so I'm re-raising this. The problem was reported as follows > I found that ubsan will report an incorrect alignment for a type in case it is allocated with the global operator new (without alignment), if we have it return an offset ptr. Full diff: https://github.com/llvm/llvm-project/pull/152532.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index c7e53331fe05f..ecc71a6343fd6 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -1798,7 +1798,7 @@ llvm::Value *CodeGenFunction::EmitCXXNewExpr(const CXXNewExpr *E) {
SkippedChecks.set(SanitizerKind::Null, nullCheck);
EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall,
E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(),
- result, allocType, result.getAlignment(), SkippedChecks,
+ result, allocType, allocAlign, SkippedChecks,
numElements);
EmitNewInitializer(*this, E, allocType, elementTy, result, numElements,
diff --git a/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp b/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp
new file mode 100644
index 0000000000000..4586a0932affd
--- /dev/null
+++ b/compiler-rt/test/ubsan/TestCases/global-new-alignment.cpp
@@ -0,0 +1,32 @@
+// RUN: %clangxx -fsanitize=alignment %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not="runtime error" -allow-empty
+// Disable with msan and tsan because they also override global new
+// UNSUPPORTED: ubsan-msan, ubsan-tsan
+
+#include <cassert>
+#include <cstddef>
+#include <cstdlib>
+
+void *operator new(std::size_t count) {
+ constexpr const size_t offset = 8;
+
+ // allocate a bit more so we can safely offset it
+ void *ptr = std::malloc(count + offset);
+
+ // verify malloc returned 16 bytes aligned mem
+ static_assert(__STDCPP_DEFAULT_NEW_ALIGNMENT__ == 16,
+ "Global new doesn't return 16 bytes aligned memory!");
+ assert((reinterpret_cast<std::ptrdiff_t>(ptr) &
+ (__STDCPP_DEFAULT_NEW_ALIGNMENT__ - 1)) == 0);
+
+ return static_cast<char *>(ptr) + offset;
+}
+
+struct Param {
+ void *_cookie1;
+ void *_cookie2;
+};
+
+static_assert(alignof(Param) == 8, "Param struct alignment must be 8 bytes!");
+
+int main() { Param *p = new Param; }
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vitalybuka
left a comment
vitalybuka
left a comment
There was a problem hiding this comment.
Added folks how had opinion on original one.
clang/lib/CodeGen/CGExprCXX.cpp
Outdated
| EmitTypeCheck(CodeGenFunction::TCK_ConstructorCall, | ||
| E->getAllocatedTypeSourceInfo()->getTypeLoc().getBeginLoc(), | ||
| result, allocType, result.getAlignment(), SkippedChecks, | ||
| result, allocType, allocAlign, SkippedChecks, |
There was a problem hiding this comment.
The PR changes clang codegen, so it needs codegen test.
Ideally precomming the test with existing wrong behaviour,
and just update the test in this PR to correct behaviour.
There was a problem hiding this comment.
To check my understanding, you mean I should do a separate pr with a test that fails due to this issue, and then fix it in this pr?
There was a problem hiding this comment.
@gbMattN CodeGen tests shouldn't fail per se. They should have checks that show what the current CodeGen output is.
i.e.,
- dump your godbolt example into a new file in clang/test/CodeGenCXX (remove the dependency on
#includes though) - add a
// RUN:command to the top and relevant// CHECK:s (see lifetime-sanitizer.cpp for an example; utils/update_cc_test_checks.py may also be helpful).
In this example, one of the checks will be something like[[TMP1]] = and i64 [[TMP2]], 15, which is checking the pointer for 16-byte alignment.
If you runLIT_FILTER=your_test_name ninja check-clang, the test should pass, because it's comparing the buggy compiler against the buggy test. - Upload the test as a separate pull request. Get it reviewed and merged.
- in this PR, the main diff for the test would probably be
[[TMP1]] = and i64 [[TMP2]], 7, showing it is checking for 8-byte alignment. The test diff makes it easy to see what the impact of the patch was.
The test should pass (this time, comparing a corrected compiler against corrected test output).
There was a problem hiding this comment.
I don't think staging it as a separate commit is necessary, but yes, it's a good development approach to write a test against the unfixed compiler that you can first verify fails the way you expect it to fail, then fix the test after you fix the compiler.
There was a problem hiding this comment.
So this is interesting, because I agree that the diagnostic message is misleading, but I'm not sure this is the right solution.
The replaceable global operator new functions are required to return a pointer that's appropriately aligned to some target-specific value. In the user's test case, they're running on x86_64 Linux, where that value is 16. So they do actually have an invalid implementation of operator new. And I'm pretty sure we do in fact emit code and optimize based on the assumption that the pointer has this target-specific alignment, even if the allocated type has a lower alignment requirement than that. So this change — which IIUC actually changes UBSan to only check for the type's actual alignment — seems pretty problematic.
Now, the diagnostic message is misleading because it's claiming that the type requires 16-byte alignment when it doesn't, but the narrow fix to that is to improve the diagnostic to talk about the requirements on global operator new when allocationAlign is greater than allocAlign, not to weaken the check.
Suggested diagnostic: something like
global operator new returned misaligned address 0x000002af8fd8, requires 16 byte alignment
Ideally, we would check whether the pointer is aligned to less than allocAlign with the current diagnostic and then do a second check for the global new alignment with the new diagnostic; I dunno if that's a good use of code size, though.
rjmccall
left a comment
rjmccall
left a comment
There was a problem hiding this comment.
(flagging that changes are requested, since I failed to do so on my earlier comment)
|
The ubsan handler for misaligned pointers contains data saying what type of check caught the misalignment. One type of check is on the constructor call, which is the check type which catches this issue with operator new. Thoughts? |
|
Instead of trying to guess, can we just add a new TypeCheckKind? |
…new returns incorrectly aligned memory
gbMattN
force-pushed
the
users/nagym/ubsan-incorrect-alignment
branch
from
0de5473 to
4714264
Compare
|
I've added a new TypeCheckKind to type mismatch that gives the user some more detailed information about the nature of the misalignment. |
clang/lib/CodeGen/CGExprCXX.cpp
Outdated
| unsigned DefaultTargetAlignment = TI.getNewAlign() / TI.getCharWidth(); | ||
| SourceManager &SM = getContext().getSourceManager(); | ||
| SourceLocation Loc = E->getOperatorNew()->getLocation(); | ||
| bool IsCustomOverload = !SM.isInSystemHeader(Loc); |
There was a problem hiding this comment.
Do we really need to check whether the new operator was defined by the user? If new returns bad alignment, that's bad no matter who wrote the implementation.
Also, you can override the global new operator while using the declaration from <new>.
There was a problem hiding this comment.
If new somehow returns bad alignment without it being a user overload, it will still be caught with the regular constructor-on error. The new error adds information specific to cases when a user has erroneously returned a smaller than allowed alignment in their overload, so I thought it best to only emit the new error in those cases. I can certainly remove some of these checks if its too overcomplicated though.
And do you mind explaining your second point some more? I don't understand it yet!
There was a problem hiding this comment.
Sorry, "override" isn't the formally correct term. Certain overloads of operator new/delete are replaceable; see https://en.cppreference.com/w/cpp/language/replacement_function.html.
There was a problem hiding this comment.
If I understand correctly, thats handled by the !SM.isInSystemHeader check, seeing if this is a user replaced call. Thats tested in the new minimum-alignment test.
There was a problem hiding this comment.
No, the declaration of the operator is still in a system header; just the definition is in user code.
There was a problem hiding this comment.
Aha, I think I understand now. If you'd prefer then, I can remove this check, and use the new check whenever the alignment of the type is less than the system default alignment.
There was a problem hiding this comment.
@efriedma-quic Made the changes we talked about at the conference
|
|
This got reverted in #166197. (The revert message doesn't say why, but we've seen the new test failing e.g. on macOS, so maybe due to that?) |
To clarify, Chrome ran into the following test failures: upstream bug: https://crbug.com/457455651 I guess the fix is to just add |
|
There is also issue when tests pass in -m32, that breaks some, and while examining the tests to try and fix this I've become unconvinced that the correct value is always printed in the new error message. There appeared to be two passing tests claiming minimum alignment were two different values, so this is going back to the drawing board a little |
8 participants
…ffset pointer
This problem and fix was originally raised on Phabricator, https://reviews.llvm.org/D116861, by belkiss. I wasn't able to find them on github or the discourse so I'm re-raising this. The problem was reported as follows