This is an archive of the discontinued LLVM Phabricator instance.

[lld/mac] Fix issues around thin archives
ClosedPublic

Authored by thakis on Nov 30 2020, 6:21 PM.

Details

Reviewers
int3
Group Reviewers
Restricted Project
Commits
rG07ab597bb035: [lld/mac] Fix issues around thin archives
Summary
  • most importantly, fix a use-after-free when using thin archives, by putting the archive unique_ptr to the arena allocator. This ports D65565 to MachO
  • correctly demangle symbol namess from archives in diagnostics
  • add a test for thin archives -- it finds this UaF, but only when running it under asan (it also finds the demangling fix)
  • make forceLoadArchive() use addFile() with a bool to have the archive loading code in fewer places. no behavior change; matches COFF port a bit better

Diff Detail

Event Timeline

thakis requested review of this revision.Nov 30 2020, 6:21 PM
thakis created this revision.
int3 added a subscriber: int3.Nov 30 2020, 7:11 PM
int3 added inline comments.
lld/MachO/Driver.cpp
306

same for the rest of the calls

lld/MachO/Symbols.cpp
22

nit: can we have a blank line between functions? my vim habits depend on it :p

lld/MachO/Symbols.h
238

why not call this toString too? since everything here is MachO anyway

lld/test/MachO/Inputs/mangled-symbols.s
1 ↗(On Diff #308511)

I'd prefer we use split-file to have this file inline in the main test file. (The tests that currently use Inputs/ were written before we had split-file.)

6–15 ↗(On Diff #308511)

can we minimize the assembly? I assume CFI directives aren't needed for demangling...

lld/test/MachO/thin-archive.s
14

is running FileCheck really necessary here if the input is going to be empty? I guess the intent is to verify that the CHECK-NOT line doesn't match, but if lld were emitting an error, this line would fail anyway...

might be clearer to just run the commands and check that out contains the symbol definition, similar to what archive.s is doing

int3 added inline comments.Nov 30 2020, 9:17 PM
lld/MachO/Driver.cpp
306

actually, I guess this would make for rather verbose code. How about making the parameter default to false instead?

thakis updated this revision to Diff 308640.Dec 1 2020, 6:37 AM
thakis marked an inline comment as done.

comments

lld/MachO/Driver.cpp
306

I think lld code likes to be explicit about arguments -- cf LinkerDriver::enqueuePath() in lld/COFF/Driver.cpp which looks very similar to this.

If you hate the bool I can invent a two-state enum for this so that it's addFile(path, kForceLoadArchive), addFile(path, kRegularFile). But given that there are just a few callers, all in one file, and COFF doesn't do it, I'm not sure it's worth it?

lld/MachO/Symbols.cpp
22

Done. Off-topic: Do you use { / } or [] / ][, [[, ]]? I like the spirit behind the latter but they don't seem to work well for me for C++ code. And { / } movements are often to granular, so I usually navigate with ctrl-f / ctrl-b / search / marks.

lld/MachO/Symbols.h
238

Because the argument is of type llvm::object::Archive::Symbol and we have toXString(llvm::object::Archive::Symbol) in COFF and ELF too (and so normal overloading based on different types in different ports doesn't work). We can't (easily) put toString() in the macho namespace since some of its overloads are in lld/Common. IIRC the change where I added toCOFFString() had some discussion about the various tradeoffs; we settled on not using toString() for the archive symbol.

lld/test/MachO/Inputs/mangled-symbols.s
1 ↗(On Diff #308511)

Yes, split-file is much nicer, I'm just not used to it. Done :)

6–15 ↗(On Diff #308511)

Done. The cfi stuff isn't needed; I just pasted the output of clang -S for void f() {}.

thakis updated this revision to Diff 308644.Dec 1 2020, 6:48 AM

missed a comment

lld/test/MachO/thin-archive.s
14

Whoops, missed this. Sure, done.

int3 accepted this revision.Dec 1 2020, 2:05 PM

lgtm

lld/MachO/Driver.cpp
306

hm fine I guess this is okay as-is

lld/MachO/Symbols.cpp
22

heh yeah I use a lot of { / }, regardless of language

This revision is now accepted and ready to land.Dec 1 2020, 2:05 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 3:48 PM
int3 added inline comments.Dec 2 2020, 9:04 PM
lld/MachO/Driver.cpp
306

actually, I do see quite a few LLD functions with default arguments... you can see a quick sample via

grep -r " = \"\")"
grep -r " = 0)"