Page MenuHomePhabricator

[asan/win] Delay load dbghelp.dll to delay ucrtbase.dll initialization
ClosedPublic

Authored by rnk on Nov 9 2016, 2:05 PM.

Details

Summary

ASan needs to initialize before ucrtbase.dll so that it can intercept all of
its heap allocations. New versions of dbghelp.dll depend on ucrtbase.dll, which
means both of those DLLs will initialize before the dynamic ASan runtime. With
delayloading, we delay the ucrtbase initialization and can intercept its
allocations.

Eventually, I would like to remove our dbghelp.dll dependency in favor of
always using llvm-symbolizer.exe, but this seems like an acceptable interim
solution.

Fixes PR30903

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 77393.Nov 9 2016, 2:05 PM
rnk retitled this revision from to [asan/win] Delay load dbghelp.dll to delay ucrtbase.dll initialization.
rnk updated this object.
rnk added a reviewer: etienneb.
rnk added a subscriber: llvm-commits.

I don't think adding those huge binary files into the repository is a good idea. Can the test use the DLLs from a system location?

rnk added a comment.Nov 9 2016, 3:31 PM
In D26473#591156, @kubabrecka wrote:

I don't think adding those huge binary files into the repository is a good idea. Can the test use the DLLs from a system location?

No, my system copy of dbghelp is not new enough to reproduce the issue. All tests fail reliably on systems with a new enough version of dbghelp. I can delete the test if you think 1MB of data is too much cost to pay for this, but we open ourselves up to regressions, as the bots probably don't have "new" dbghelp.

rnk added a comment.Nov 9 2016, 3:32 PM

Also, this isn't sufficient to fix the issue in static builds. I'm rewriting this to use LoadLibrary / GetProcAddress.

rnk updated this revision to Diff 77419.Nov 9 2016, 4:09 PM
  • Use LoadLibrary instead of /delayload:dbghelp.dll
rnk updated this revision to Diff 77420.Nov 9 2016, 4:14 PM
  • Remove stray header

Ok, now that you've removed the dependency on dbghelp.dll, is the test (and the binary files) still needed? I mean, now, it doesn't matter what dbghelp.dll depends on.

rnk added a comment.Nov 10 2016, 1:36 PM
In D26473#591256, @kubabrecka wrote:

Ok, now that you've removed the dependency on dbghelp.dll, is the test (and the binary files) still needed? I mean, now, it doesn't matter what dbghelp.dll depends on.

I don't really see how the code change affects the usefulness of the test. You seem strongly opposed to having a few test binaries in the repo, though. Maybe a better test would be to ensure that the dynamic asan runtime depends only on a whitelist of DLLs, such as kernel32 et al.

In D26473#592220, @rnk wrote:

Maybe a better test would be to ensure that the dynamic asan runtime depends only on a whitelist of DLLs, such as kernel32 et al.

That does sounds like a better test.

aaron.ballman accepted this revision.Nov 10 2016, 4:09 PM
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a subscriber: aaron.ballman.

Modulo @kubabrecka 's comments, LGTM

lib/sanitizer_common/sanitizer_dbghelp.h
28 ↗(On Diff #77420)

I love this pattern. We should clean up some of LLVM's header similarly.

lib/sanitizer_common/sanitizer_symbolizer_win.cc
67 ↗(On Diff #77420)

Should we also assert that the returned pointer is not null?

This revision is now accepted and ready to land.Nov 10 2016, 4:09 PM
rnk updated this revision to Diff 77573.Nov 10 2016, 4:23 PM
rnk edited edge metadata.
  • Rewrite test with llvm-readobj
  • CHECK our dbghelp imports
etienneb accepted this revision.Nov 14 2016, 8:03 AM
etienneb edited edge metadata.
This revision was automatically updated to reflect the committed changes.