[lsan] Fix free(NULL) interception during initialization #106912
Conversation
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: None (tmiasko) ChangesPreviously an attempt to free a null pointer during initialization would fail on ENSURE_LSAN_INITED assertion (since a null pointer is not owned by DlsymAlloc). Full diff: https://github.com/llvm/llvm-project/pull/106912.diff 1 Files Affected:
diff --git a/compiler-rt/lib/lsan/lsan_interceptors.cpp b/compiler-rt/lib/lsan/lsan_interceptors.cpp
index b569c337e97641..db27be7d06995f 100644
--- a/compiler-rt/lib/lsan/lsan_interceptors.cpp
+++ b/compiler-rt/lib/lsan/lsan_interceptors.cpp
@@ -77,6 +77,8 @@ INTERCEPTOR(void*, malloc, uptr size) {
}
INTERCEPTOR(void, free, void *p) {
+ if (!p)
+ return;
if (DlsymAlloc::PointerIsMine(p))
return DlsymAlloc::Free(p);
ENSURE_LSAN_INITED;
|
| @@ -77,6 +77,8 @@ INTERCEPTOR(void*, malloc, uptr size) { | |||
| } | |||
|
|
|||
| INTERCEPTOR(void, free, void *p) { | |||
| if (!p) | |||
There was a problem hiding this comment.
this will fix only lsan
to fix all, I propose to change
DlsymAlloc::PointerIsMine to return true for NULL
and teach DlsymAlloc::Free to handle it correctly
There was a problem hiding this comment.
Indeed there are a few more sanitizers that missed this special case.
I would suggest fixing those by considering NULL, since DlsymAlloc owning NULL would be counterproductive for realloc.
There was a problem hiding this comment.
I see, you are correct.
Can you please include all other sanitizers here, also cfree, and please use UNLIKELY(!p)
There was a problem hiding this comment.
I updated the patch to include UNLIKELY in the condition.
For lsan cfree interceptor is aliased to free and doesn't need further changes.
I looked a little bit more at other sanitizers and I don't think they actually need any further changes. For those that don't immediately return for null, it is fine if execution just falls through to the main deallocation function, since null case is handled early on there.
There was a problem hiding this comment.
Please rebase and include into the patch removal of XFAIL added in #108289
rust-cloud-vms
bot
force-pushed
the
lsan-free-null
branch
from
ab1444f to
6bbc9f9
Compare
Almost all sanitizers already support the test. * Tsan does not use DlsymAlloc yet. * Lsan will support with #106912. memprof,rtsan,nsan are not tested as part of sanitizer_common, but we should keep them here to show up when it happen. --------- Co-authored-by: Xiaofeng Tian <110771974+txff99@users.noreply.github.com>
vitalybuka
force-pushed
the
lsan-free-null
branch
from
6bbc9f9 to
9d1db3d
Compare
Previously an attempt to free a null pointer during initialization would fail on ENSURE_LSAN_INITED assertion (since a null pointer is not owned by DlsymAlloc).
rust-cloud-vms
bot
force-pushed
the
lsan-free-null
branch
from
9d1db3d to
d9157c5
Compare
|
Thanks. Can you merge this for me? |
Will do! |
Almost all sanitizers already support the test. * Tsan does not use DlsymAlloc yet. * Lsan will support with llvm#106912. memprof,rtsan,nsan are not tested as part of sanitizer_common, but we should keep them here to show up when it happen. --------- Co-authored-by: Xiaofeng Tian <110771974+txff99@users.noreply.github.com>
Previously an attempt to free a null pointer during initialization would fail on ENSURE_LSAN_INITED assertion (since a null pointer is not owned by DlsymAlloc).
|
this seems to have broken the test on mac: https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/2058/ |
With #106912, the test now fails on macos, e.g. https://green.lab.llvm.org/job/llvm.org/job/clang-stage1-RA/2058/.
Previously an attempt to free a null pointer during initialization would fail on ENSURE_LSAN_INITED assertion (since a null pointer is not owned by DlsymAlloc).