Skip to content

Work-around / fix libasan incompatibility #389

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 8, 2022
Merged

Work-around / fix libasan incompatibility #389

merged 4 commits into from
May 8, 2022

Conversation

psychon
Copy link
Contributor

@psychon psychon commented May 4, 2022

This is my attempt at fixing #365. I cleaned up the proposed patch from there and actually simplified it a bit. I also added a unit test that would previously hang (deadlock in pthread_mutex_lock()) and now passes.

As requested, this adds a new define. I chose to call it FAIL_PRE_INIT_CALLS. I also had to enable it by default since otherwise the test would hang. I kind of expect this to be an unsatisfactory solutions. Please advice how to properly integrate this test (or whether it should just be deleted).

I did test that this makes the hang with both clang -fsanitize=address main.c and clang++ -fsanitize=address main.cpp go away (where main.c(pp) is just an empty main function). I also checked that without this changes, the resulting programs would hang under faketime.

Fixes #365

Edit: Also, I was lazy and just failed all calls before ftpl_init(), not only during ftpl_init(). That's different to the previous hacky patch.

psychon added 2 commits May 4, 2022 14:25
Backtraces suggest that AddressSanitizer replaces malloc() with a
function that

- locks a mutex and
- calls clock_gettime() while the mutex is held

This commit adds a test that implements a trivial malloc() that behaves
similarly. Currently, this test hangs.

Signed-off-by: Uli Schlachter <[email protected]>
This commit adds a new define FAIL_PRE_INIT_CALLS. When that define is
set, calls to clock_gettime() that occur before ftpl_init() was called
(due to being marked with __attribute__((constructor))) will just fail
and return -1.

After this commit, the test case added in the previous commit no longer
hangs. To make this actually work, this new define is enabled by
default.

Fixes: #365
Signed-off-by: Uli Schlachter <[email protected]>
@wolfcw wolfcw self-assigned this May 5, 2022
@wolfcw
Copy link
Owner

wolfcw commented May 5, 2022

Thanks, that looks simple and good so far. :-)

I'd suggest to also add FAIL_PRE_INIT_CALLS optionally to test/Makefile and adjust libmallocintercept.c accordingly, so the test does nothing / not hang when the new define is not set.

Unless we notice other resulting issues, it might be a good thing to enable it by default though.

@psychon
Copy link
Contributor Author

psychon commented May 8, 2022

I'd suggest to also add FAIL_PRE_INIT_CALLS optionally to test/Makefile

That means users have to edit two Makefiles (or use an env var), right? Fine with me. I'll push a commit doing that.

Unless we notice other resulting issues, it might be a good thing to enable it by default though.

The "old" code also managed to fake time before constructors were called. With my change, things now depend on I-dont-know-what and "early" calls would get no time (and nothing handles failures of clock_gettime). So... something "out there" might break.

I disabled it by default.

make check now prints lots of stuff like:

Called free() from libmallocintercept...FAIL_PRE_INIT_CALLS not defined, skipping poke_faketime() successfully

make clean test FAKETIME_COMPILE_CFLAGS=-DFAIL_PRE_INIT_CALLS does what it did before. (And I also fixed make clean to also delete libmallocintercept.so)

@wolfcw wolfcw merged commit 141d1a7 into wolfcw:master May 8, 2022
@psychon psychon deleted the asan branch May 8, 2022 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deadlock when address sanitizer is used under clang since v0.9.7
2 participants