This is an archive of the discontinued LLVM Phabricator instance.

Avoid UAF in WinCOFFObjectWriter with weak symbols.
ClosedPublic

Authored by maleadt on Jul 15 2022, 1:46 AM.

Details

Summary

When using weak symbols, the WinCOFFObjectWriter keeps a list (WeakDefaults)
that's used to make names unique. This list should be reset when the object
writer is reset, because otherwise reuse of the object writer can result in
freed symbols being accessed. With some added output, this becomes clear when
using llc in --run-twice mode:

$ ./llc --compile-twice -mtriple=x86_64-pc-win32 trivial.ll -filetype=obj

DefineSymbol::WeakDefaults
 - .weak.foo.default
 - .weak.bar.default

DefineSymbol::WeakDefaults
 - .weak.foo.default
 - áÑJij⌂  p§┼Ø┐☺
 - .debug_macinfo.dw
 - .weak.bar.default

This does not seem to leak into the output object file though, so I couldn't
come up with a test. I added one that just does --run-twice (and verified
that it does access freed memory), which should result in detecting the
invalid memory accesses when running under ASAN.

Observed in a Julia PR where we started using weak symbols:
https://github.com/JuliaLang/julia/pull/45649

Diff Detail

Event Timeline

maleadt created this revision.Jul 15 2022, 1:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 1:46 AM
maleadt requested review of this revision.Jul 15 2022, 1:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2022, 1:46 AM
maleadt added a project: Restricted Project.Jul 15 2022, 1:47 AM
maleadt added a reviewer: mstorsjo.
mstorsjo accepted this revision.Jul 15 2022, 6:18 AM

LGTM, thanks! Do you have commit access, or do you want me to push it for you? In that case, can you provide your preferred git author line (Real Name <email@address>)?

This revision is now accepted and ready to land.Jul 15 2022, 6:18 AM
maleadt planned changes to this revision.Jul 15 2022, 7:08 AM

Looks like I botched the test here (missing a RUN directive), so let me update that first.

I don't have commit access; you can use Tim Besard <tim.besard@gmail.com>.

maleadt updated this revision to Diff 444974.Jul 15 2022, 7:11 AM

Fix test.

This revision is now accepted and ready to land.Jul 15 2022, 7:11 AM
maleadt updated this revision to Diff 445017.Jul 15 2022, 9:22 AM

Fix tests again.

This revision was automatically updated to reflect the committed changes.