Page MenuHomePhabricator

[libc++][libc++abi] Don't provide new/delete when built with ASan, HWASan or TSan
AbandonedPublic

Authored by phosek on Apr 2 2019, 8:40 PM.

Details

Summary

ASan, HWASan and TSan provide their own operator new/delete to intercept
all allocations and deallocations. Currently, they rely on overriding
weak symbols that are already provided by libc++abi and libc++, but that
solution is fragile and breaks in certain scenarios, e.g. when libc++abi
is statically linked into a shared libc++.

Rather we should avoid providing new/delete operators when built with
ASan, HWASan and TSan altogether and rely on new/delete operators
provided by the sanitizer runtimes.

Diff Detail

Event Timeline

phosek created this revision.Apr 2 2019, 8:40 PM
mcgrathr accepted this revision.Apr 2 2019, 8:44 PM
mcgrathr added a subscriber: mcgrathr.

lgtm

libcxxabi/src/stdlib_new_delete.cpp
268

Comment the conditions

This revision is now accepted and ready to land.Apr 2 2019, 8:44 PM
phosek updated this revision to Diff 193427.Apr 2 2019, 8:50 PM
phosek marked an inline comment as done.
eugenis accepted this revision.Apr 3 2019, 2:31 PM

LGTM

ldionne requested changes to this revision.Apr 3 2019, 2:34 PM

The right thing to do seems to be to provide -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF when you configure libc++.

This revision now requires changes to proceed.Apr 3 2019, 2:34 PM

The right thing to do seems to be to provide -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF when you configure libc++.

What I mean here is that I don't see the justification for hardcoding this into the sources. Similarly, I find it wrong for this to be dependent on _LIBCPP_ABI_VCRUNTIME and __GLIBCXX__.

phosek updated this revision to Diff 193616.Apr 3 2019, 3:45 PM
phosek added a comment.Apr 3 2019, 3:55 PM

The right thing to do seems to be to provide -DLIBCXX_ENABLE_NEW_DELETE_DEFINITIONS=OFF when you configure libc++.

What I mean here is that I don't see the justification for hardcoding this into the sources. Similarly, I find it wrong for this to be dependent on _LIBCPP_ABI_VCRUNTIME and __GLIBCXX__.

I've changed the implementation to make those independent. D60235 is the alternative that relies entirely on CMake and only disables those for Fuchsia. IMO the problem with that solution is that anyone who builds libc++/libc++abi will need to apply the same solution. I was also thinking about changing the CMake option default, but there's no easy way to detect whether the sanitizer is being enabled e.g. through CFLAGS/LDFLAGS.

I think we're way, way out on the bell curve here.
People who are building their own libc++ and statically linking it with libc++abi and using the sanitizers.
Those people (I believe) will be able to set a CMake flag.

I think I prefer the cmake flag, too.
It's not unthinkable that hwasan might stop providing operator new and friends in the future. I was a bit surprised that we even do it now in COMPILER_RT_HWASAN_WITH_INTERCEPTORS=OFF configuration - but it looks like we are keeping it.
Anyway, lets not hardcode this decision.

phosek abandoned this revision.Apr 3 2019, 4:51 PM

SGTM