This is an archive of the discontinued LLVM Phabricator instance.

Introduce new `disable_init` ASan option that is only supported on platforms where `SANITIZER_SUPPORTS_DISABLED_INIT` is true. Currently this is only supported on Darwin.
ClosedPublic

Authored by delcypher on Nov 13 2018, 4:40 AM.

Details

Summary

The purpose of this option is provide a way for the ASan dylib
to be loaded via dlopen() without triggering most initialization
steps (e.g. shadow memory set up) that normally occur when the
ASan dylib is loaded.

This new functionality is exposed by

  • A SANITIZER_SUPPORTS_INIT_FOR_DLOPEN macro which indicates if the feature is supported. This only true for Darwin currently.
  • A HandleDlopenInit() function which should return true if the library is being loaded via dlopen() and SANITIZER_SUPPORTS_INIT_FOR_DLOPEN is supported. Platforms that support this may perform any initialization they wish inside this function.

Although disabling initialization is something that could potentially
apply to other sanitizers it appears to be unnecessary for other
sanitizers so this patch only makes the change for ASan.

rdar://problem/45284065

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Nov 13 2018, 4:40 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptNov 13 2018, 4:40 AM
krytarowski added inline comments.Nov 13 2018, 5:05 AM
lib/asan/asan_flags.inc
164 ↗(On Diff #173832)

Why to add a flag for it? As far as I understand it, we require this option to workaround some issue on Darwin and it's not needed so far by others. If I understanding removal of this property will break startup on Darwin.

If so, shouldn't this be just Darwin specific hardcoded property rather than generalized and unsupported option for everybody?

lib/asan/asan_internal.h
117 ↗(On Diff #173832)

Shouldn't we add 'static inline'? Some compilers may complain that there is no prototype for a function.

delcypher added inline comments.Nov 13 2018, 5:42 AM
lib/asan/asan_flags.inc
164 ↗(On Diff #173832)

We need a flag because we still need ASan's init to behave as it did previously in most scenarios (i.e. disable_init=0). It is only in the case where we want to perform dlopen() on the ASan dynamic library that we need to set disable_init=1.

Put another way, the problem we are trying to fix cannot be hard-coded because it is a dynamic property (how the ASan library is being loaded). I've made supporting this feature a static property guarded on SANITIZER_SUPPORTS_DISABLED_INIT which is only true on Darwin.

lib/asan/asan_internal.h
117 ↗(On Diff #173832)

To be honest I'd rather stick the definition for non-Darwin platforms for in a .cc file but I wasn't sure where to put it so I just left it here.
If there isn't a better home for the non-Darwin implementation it I'll be happy to add static inline.

There is also start_deactivated in ASan.

It would help to show the crash example/log without this patch, in order to get more of the context.

Can this be triggered by a separate environment variable instead? That way we don't have to expose it in flags at all.

There is also start_deactivated in ASan.

That's a different feature. That allows ASan to be partially initialized and then the rest of the initialization is delayed until later. I believe this exists for Android.

It would help to show the crash example/log without this patch, in order to get more of the context.

The test case shows how to trigger the crash today (see not --crash ... line). The context here is we are looking for a way to make it possible to dlopen() the ASan library from a non-asanified process so that specific functions inside it can be called. Today this isn't possible.

Can this be triggered by a separate environment variable instead? That way we don't have to expose it in flags at all.

Sure. My original patch actually did this with a SanitizeInitIsDisabled() function which was platform specific. I thought moving it into flags was a bit cleaner and it also fixed a problem because some of the sanitizer's code doesn't behave that well if you skip parsing the flags.

delcypher updated this revision to Diff 174031.Nov 14 2018, 6:36 AM
  • Rename macro and support functions to explicitly mention dlopen.
  • Introduce InitIsViaDlopen() function and remove the disable_init ASan option.
kubamracek added inline comments.Nov 14 2018, 6:56 AM
lib/asan/asan_rtl.cc
403 ↗(On Diff #174031)

Change this to print only under verbose mode.

lib/sanitizer_common/sanitizer_platform.h
348–352 ↗(On Diff #174031)

Why do we need this? I'd remove it.

delcypher updated this revision to Diff 174050.Nov 14 2018, 8:49 AM

Only report dlopen init when verbosity is non-zero.

delcypher marked an inline comment as done.Nov 14 2018, 8:49 AM
delcypher added inline comments.
lib/sanitizer_common/sanitizer_platform.h
348–352 ↗(On Diff #174031)

Two reasons:

  1. I wanted the feature to be zero-cost for platforms that don't use it. You can see it's used in the if condition in AsanInitInternal(). It's a compile time constant so for platforms that don't support this feature the branch should just be folded away and so it won't even be checked at runtime.
  2. We need to something to let us switch between the definitions of InitIsViaDlopen() and HandleDlopenInit(). This macro is used for this.

If you have a clean solution to handle the second reason without SANITIZER_SUPPORTS_INIT_FOR_DLOPEN then I can implement it.

kubamracek added inline comments.Nov 14 2018, 8:54 AM
lib/asan/asan_rtl.cc
404 ↗(On Diff #174050)

Use VReport instead.

kubamracek added inline comments.Nov 14 2018, 8:56 AM
lib/sanitizer_common/sanitizer_platform.h
348–352 ↗(On Diff #174031)

We're trying to minimize compile-time constants, as that adds complexity. Why not just have this behavior (reading an env var to skip initialization) on all platforms?

delcypher updated this revision to Diff 174051.Nov 14 2018, 8:58 AM

Use VReport.

delcypher marked an inline comment as done.Nov 14 2018, 8:58 AM
delcypher added inline comments.
lib/asan/asan_rtl.cc
404 ↗(On Diff #174050)

Good call. I forgot about that macro.

delcypher marked an inline comment as done.Nov 14 2018, 8:59 AM
delcypher added inline comments.Nov 14 2018, 9:46 AM
lib/sanitizer_common/sanitizer_platform.h
348–352 ↗(On Diff #174031)

I don't think having this behaviour an all platforms is a good idea. Our implementation is Darwin specific and I don't think other platforms should have to care about this at all. I think it should be a platform maintainer's choice to allow/disallow disabling of the ASan init process. Given that I'm adding a new feature that is only relevant to Darwin right now I think the feature should be completely non-existent on other platforms. This is why I chose to introduce the SANITIZER_SUPPORTS_INIT_FOR_DLOPEN compile time constant.

Also if we were to make this feature work on all platforms then it would make more sense to put the option in ASAN_OPTIONS (where it was originally in this patch) which you asked me not to do.

kcc added a comment.Nov 14 2018, 10:56 AM

I'd like Evgenii to take a look, since there is a similarity with what we do in Android.

kcc added a comment.Nov 14 2018, 10:59 AM

I also don't understand how it works.
What happens if you do this

x = malloc()
dlopen("asan.so");
free(x)

This needs to be a) covered by test and b) explained in comments.

lib/asan/asan_internal.h
114 ↗(On Diff #174051)

please avoid introducing new ifdefs. There are plenty of them already, but no reason to make things worse.

Is this needed for the clients of that out-of-process allocator inspection thing? Could it be a separate library that links asan allocator in out-of-process mode, but not the rest? Otherwise you are adding a mode of execution to ASan when there is no shadow mapping, and now that's something that everyone else have to keep in mind.

In D54469#1298802, @kcc wrote:

I also don't understand how it works.
What happens if you do this

x = malloc()
dlopen("asan.so");
free(x)

This needs to be a) covered by test and b) explained in comments.

When the ASan library gets loaded we don't want any of the interceptors to be used. The actual purpose of being to load the ASan library this way is simply so we gain access to the ASan malloc enumerator implementation.
So in the example above, both malloc() and free() are supposed to be using the same system allocator.

We want to enable doing this because this is how the memory analysis tools on macOS work. They freeze a target process, dlopen the allocator implementation that the target process is using and then call functions in the dlopen()'ed code to perform out-of-process enumeration on behalf of the analysis tool.

I can can extend the existing test to as you suggest and add comments to explain what we expect to happen here.

kcc added a comment.Nov 14 2018, 12:30 PM

When the ASan library gets loaded we don't want any of the interceptors to be used. The actual purpose of being to load the ASan library this way is simply so we gain access to the ASan malloc enumerator implementation.

I am more confused now.
What exactly do you call the "ASan malloc enumerator"?
Why is it going to work when the allocations come from a non-asan allocator?

Is this needed for the clients of that out-of-process allocator inspection thing?

Yes, this is exactly what this patch is for.

Could it be a separate library that links asan allocator in out-of-process mode, but not the rest? Otherwise you are adding a mode of execution to ASan when there is no shadow mapping, and now that's something that everyone else have to keep in mind.

This is something we have planned as a later patch (once all of the enumeration pieces have landed). However we also want to support dlopen() of the full ASan shared library for various deployment scenarios that we are concerned with.
I was hoping to make this patch as non-intrusive as possible so that other platforms need not worry about the special mode that only exists on Darwin. With the patch as it stands right now other platforms just need to make sure they don't add stuff before
if (SANITIZER_SUPPORTS_INIT_FOR_DLOPEN && UNLIKELY(InitIsViaDlopen())) {... } . Is that asking too much of other platform maintainers? If yes, how would suggest we modify this patch to make it easier for other platforms to ignore the existence of this Darwin only feature?

delcypher added a comment.EditedNov 14 2018, 1:06 PM
In D54469#1298884, @kcc wrote:

When the ASan library gets loaded we don't want any of the interceptors to be used. The actual purpose of being to load the ASan library this way is simply so we gain access to the ASan malloc enumerator implementation.

I am more confused now.

Sorry. I will try to explain.

What exactly do you call the "ASan malloc enumerator"?

The "ASan malloc enumerator" in this context means the code that exists in the ASan library that knows how the enumerate the allocations of another process that is using ASan's allocator, i.e. this is the out-of-process allocator enumeration implementation that we are currently trying to land in https://reviews.llvm.org/D53975 .

Why is it going to work when the allocations come from a non-asan allocator?

Because the intention (when all the ASan malloc enumerator patches land) is that when we call functions in the dlopen()-ed ASan shared library, we will be asking it inspect a different process (one that is using ASan for all allocations), not ourselves.
This is the model that memory analysis tools on Darwin follow. Let's say you have a program foo_asan running that is built with ASan and you want to use the Darwin leaks programs to inspect it. When you launch leaks and tell it to inspect foo_asan it would do the following.

  1. Freeze foo_asan so that all threads stop rusnning (similar to LSan's stop-the-world)
  2. leaks then examines the foo_asan process and figures out what allocator implementation is being used. In this case it is ASan.
  3. leaks then calls dlopen() on the ASan shared library that foo_asan is using.
  4. leaks then finds a special function inside the ASan shared library that knows how to enumerate the allocations of a process using ASan.
  5. leaks calls this special function, telling it to examine the frozen foo_asan program on its behalf.
  6. After the special function is finished, leaks then unfreezes foo_asan allowing it to continue executing.

What this patch is doing is enabling step 3., in this list. Making it possible to dlopen() the ASan shared library without triggering its usual initialisation process because the usual initialisation process would abort the leaks process.

This patch doesn't explicitly mention any of this because the feature is adding is not specific to ASan allocator enumeration at all. It is simply trying to make it possible to load the ASan shared library via dlopen() and call some functions inside it. Our end goal is of course to get out-of-process ASan allocator enumeration working. However knowing this is not required to understand what this patch is doing. Of course knowing this answers why we are trying to add this feature.

Does this make sense now?

delcypher added inline comments.Nov 14 2018, 1:10 PM
lib/asan/asan_internal.h
114 ↗(On Diff #174051)

Okay. I will try to just declare these functions without the macro guards and just put stub implementations in each of the platform specific files.

delcypher updated this revision to Diff 174184.Nov 15 2018, 4:10 AM

Remove #ifdefs around introduced functions and instead implement each of them
in the platform specific files.

delcypher marked 2 inline comments as done.Nov 15 2018, 4:18 AM
delcypher added inline comments.
lib/asan/asan_internal.h
114 ↗(On Diff #174051)

@kcc I've tried to remove the #ifdef by always declaring the new functions and implementing each of them for each of the ASan supported platforms.

delcypher marked an inline comment as done.Nov 19 2018, 4:36 AM

@kubamracek @kcc It seems this review has stalled (probably due to Thanksgiving). Any more thoughts on this?

kcc added inline comments.Nov 28 2018, 12:49 PM
lib/asan/asan_internal.h
115 ↗(On Diff #174184)

I wonder if you can turn these two functions into one.

delcypher added inline comments.Nov 29 2018, 8:54 AM
lib/asan/asan_internal.h
115 ↗(On Diff #174184)

Sure it's possible. I'll have a go at doing this and will update the patch shortly.

delcypher updated this revision to Diff 175886.Nov 29 2018, 9:33 AM

Merge InitIsViaDlopen() and HandleDlopenInit() into a single function
named HandleDlopenInit().

delcypher updated this revision to Diff 175889.Nov 29 2018, 9:36 AM

Fix mistake in Darwin implementation of HandleDlopenInit() where we never actually
did the initialization.

delcypher marked 2 inline comments as done.Nov 29 2018, 9:38 AM
delcypher added inline comments.
lib/asan/asan_internal.h
115 ↗(On Diff #174184)

@kcc Done.

delcypher edited the summary of this revision. (Show Details)Nov 29 2018, 9:38 AM
kcc accepted this revision.Nov 30 2018, 4:32 PM

LGTM

This revision is now accepted and ready to land.Nov 30 2018, 4:32 PM
This revision was automatically updated to reflect the committed changes.