Page MenuHomePhabricator

[winasan] Pin the ASan DLL to prevent unloading
ClosedPublic

Authored by dmajor on Sep 25 2018, 9:10 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

dmajor created this revision.Sep 25 2018, 9:10 AM

@rnk, do I guess correctly that the runtime pretty deeply assumes that it will never be unloaded? Interceptions, exception handlers, etc. never get cleaned up? If that's the case then I don't feel too bad about this hack. But if unloading is supported by the design, then maybe we should do something more graceful than this.

rnk accepted this revision.Sep 25 2018, 2:46 PM

Looks good, but consider updating the comment.

lib/asan/asan_win.cc
296–297 ↗(On Diff #166931)

I would reword this a bit to not imply that we should remove the handler. Basically, ASan can't ever reinitialize itself. It can't unmap its shadow memory and remove its handler because some instrumented code in some other DLL might touch the shadow memory. If the memory hasn't yet been mapped and there is no exception handler, it will crash, or if it has and we unmap the memory, the app will crash.

This revision is now accepted and ready to land.Sep 25 2018, 2:46 PM
dmajor added inline comments.Sep 25 2018, 2:56 PM
lib/asan/asan_win.cc
296–297 ↗(On Diff #166931)

So, I don't think _that_ particular scenario is a concern, because any instrumented DLL still loaded would keep a ref on the ASan DLL.

The situation where we get into trouble is, if all instrumented binaries get unloaded and drop the ASan DLL's refcount to zero, and then some unrelated exception happens (that ShadowExceptionHandler would have just passed through) we jump into bad memory, which itself causes another entry into ShadowExceptionHandler, over and over until the stack runs out.

In any case, good to know that the design is never to be unloaded. I'll make the wording more generic. Thanks!

dmajor updated this revision to Diff 167022.Sep 25 2018, 3:49 PM

After writing the last comment and thinking about this some more, I convinced myself that this is more of a problem with interceptors than exception handlers.

In theory we might be able to react to DllUnload and remove ShadowExceptionHandler, but removing the interceptors would be harder, since we don't keep a list of them.

And, the hooks could bite us even on 32-bit where ShadowExceptionHandler doesn't exist, if all the ASan binaries go away and some non-ASan binary tries to call ntdll!memset or whatever.

So I moved the patch to InitializePlatformInterceptors. What do you think?

dmajor requested review of this revision.Sep 25 2018, 3:50 PM
rnk accepted this revision.Sep 25 2018, 5:19 PM

Right, that's probably a better rationale. 👍

This revision is now accepted and ready to land.Sep 25 2018, 5:19 PM
This revision was automatically updated to reflect the committed changes.