Page MenuHomePhabricator

[sanitizer] Bail out with warning if user dlopens shared library with RTLD_DEEPBIND flag
ClosedPublic

Authored by m.ostapenko on Mar 1 2017, 9:50 AM.

Details

Summary

People keep hitting on spurious failures in malloc/free routines when using sanitizers with shared libraries dlopened with RTLD_DEEPBIND (see https://github.com/google/sanitizers/issues/611 for details).
Let's check for this flag and bail out with warning message instead of failing in random places.

Diff Detail

Repository
rL LLVM

Event Timeline

m.ostapenko created this revision.Mar 1 2017, 9:50 AM
ygribov added inline comments.Mar 1 2017, 10:21 AM
lib/sanitizer_common/sanitizer_common.cc
485 ↗(On Diff #90200)

Does it make sense to print backtrace?

test/sanitizer_common/TestCases/Posix/deepbind.cc
1 ↗(On Diff #90200)

Do you need shlib at all? You don't care if dlopen succeeds so can call it with arbitrary path.

4 ↗(On Diff #90200)

Android does not have it?

18 ↗(On Diff #90200)

You can just return 0 after this.

kubamracek added inline comments.Mar 1 2017, 10:57 AM
lib/sanitizer_common/sanitizer_common.cc
480 ↗(On Diff #90200)

This will not compile on macOS (RTLD_DEEPBIND is not defined there).

m.ostapenko added inline comments.Mar 1 2017, 11:59 AM
lib/sanitizer_common/sanitizer_common.cc
480 ↗(On Diff #90200)

Thanks, will move this to sanitizer_linux. cc.

485 ↗(On Diff #90200)

Perhaps. At least we should print library name here.

test/sanitizer_common/TestCases/Posix/deepbind.cc
1 ↗(On Diff #90200)

Just wanted to make sure that without deepbind everything works as expected. I can drop this for sure.

4 ↗(On Diff #90200)

AFAIK Android doesn't intercept dlopen.

m.ostapenko marked 9 inline comments as done.Mar 2 2017, 12:11 AM
m.ostapenko added inline comments.
lib/sanitizer_common/sanitizer_common.cc
485 ↗(On Diff #90200)

sanitizer_common doesn't have unified stacktrace printing routine (e.g. GET_STACK_TRACE_FATAL), every tool defines it by itself. Thus I don't print backtrace here to avoid unnecessary complexity.

vitalybuka accepted this revision.Mar 6 2017, 3:36 PM
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_linux.cc
1472 ↗(On Diff #90291)

I'd prefer we have flags to ignore this. But according kcc@ on the bug it's overkill, so LGTM

This revision is now accepted and ready to land.Mar 6 2017, 3:36 PM
This revision was automatically updated to reflect the committed changes.

We have a test in chromium that loads a small so file and calls some of the functions therein. There are no allocations in the so file. The test used to run fine under tsan, now it fails with the error message you added. I'll disable the test under tsan, but maybe others out there where also calling non-allocating so's without problems before this change.

We have a test in chromium that loads a small so file and calls some of the functions therein. There are no allocations in the so file. The test used to run fine under tsan, now it fails with the error message you added. I'll disable the test under tsan, but maybe others out there where also calling non-allocating so's without problems before this change.

Oh, sorry. Perhaps we can remove Die()? The check still should fire a warning IMHO.