Page MenuHomePhabricator

[lld-macho] Fix dangling string reference when adding frameworks
ClosedPublic

Authored by PRESIDENT810 on Oct 13 2021, 3:49 AM.

Details

Summary

In Driver.cpp, addFramework used std::string instance to represent the path of a framework, which will be freed after the function returns. However, this string is stored in loadedArchive, which will be used later to compare with path of newly added frameworks. This caused https://bugs.llvm.org/show_bug.cgi?id=52133. A test is included in this commit to reproduce this bug.

Now resolveDylibPath returns a StringRef instance, and it uses StringSaver to save its data, then returns it to functions on the top. This ensures the resolved framework path is still valid after LC_LINKER_OPTION is parsed.

Diff Detail

Event Timeline

PRESIDENT810 created this revision.Oct 13 2021, 3:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
PRESIDENT810 requested review of this revision.Oct 13 2021, 3:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 13 2021, 3:49 AM

.... and if the StringRef instance's actual string data is released, such lookup will return false even though the value is still present in the map ....

Hmmm, do we have a use-after-free bug somewhere?
IUC, all the StringRef should have their data "saved" in the saver and none of it should get freed while we're still linking

Could you add a test to reproduce this?

Add a test case to illustrate where a framework (archive) is loaded twice.

oontvoo added inline comments.Oct 14 2021, 8:31 AM
lld/test/MachO/multi-loaded-archive.yaml
8–11 ↗(On Diff #379683)

Could you make sure any any file produced by this test is under %t/ ? our test machines limit where the tests can write

(eg., the main.out should be %t/main.out, similarly, any implicit "current" dir should be spelled out to "%t" when possible)

Thanks!

oontvoo added inline comments.Oct 14 2021, 5:54 PM
lld/MachO/Driver.cpp
355–357 ↗(On Diff #379683)

I think this is where the dangling reference is .

Could you add saver.save(*path) here?
(ie.,addFile(saver.save(*path),/*forceLoadArchive=*/false, isExplicit)) )

Storing the strings themselves are probably better than hash_code.

oontvoo added inline comments.Oct 14 2021, 7:11 PM
lld/MachO/Driver.cpp
355–357 ↗(On Diff #379683)

P.S: Or perhaps the more general fix is to make resolveDylibPath[0] return an Optional<StringRef> (where it already "saves" the string's content into the saver).

[0] https://github.com/llvm/llvm-project/blob/d3cb6bf2d462958f2a6573e6390cca71d13dee6b/lld/MachO/DriverUtils.cpp#L186

oontvoo requested changes to this revision.Oct 15 2021, 8:28 AM

[clearing queue]

This revision now requires changes to proceed.Oct 15 2021, 8:28 AM
PRESIDENT810 retitled this revision from [lld-macho] Use StringRef's hash code as keys of loadedArchives to [lld-macho] Fix dangling string reference when adding frameworks.

Hello.

I added %t to paths in my test, and they should be ok now. I also changed a few functions related to adding frameworks, so now they return StringRef instances instead of std::string, and their values are all saved by saver.save(). In this way I think we don't need to change the key of loadArchives to hash_code.

PRESIDENT810 edited the summary of this revision. (Show Details)Oct 18 2021, 1:46 AM

Hello.

I added %t to paths in my test, and they should be ok now. I also changed a few functions related to adding frameworks, so now they return StringRef instances instead of std::string, and their values are all saved by saver.save(). In this way I think we don't need to change the key of loadArchives to hash_code.

Thanks! Looks good!
One last request, can you make the test use simplified asm (rather than yaml) ? The yaml format is kind of hairy and hard to maintain ... it's only used when the obj files cannot be produced any other way.
For this test, I don't think it's necessary to use it.
(There are other similar test in simplified asm, you may see https://github.com/llvm/llvm-project/tree/main/lld/test/MachO for egs).

lld/MachO/Driver.cpp
116 ↗(On Diff #380307)

I think this could be resolveDylibPath(saver.save(symlink.str())

Then in resolveDylibPath (ie., DriverUtils.cpp:line 199), you don't save the StringRef because the assumption is that when you see a StringRef, it's already been saved.

lld/MachO/InputFiles.cpp
916 ↗(On Diff #380307)

pls run clang-format on patch

917 ↗(On Diff #380307)

this should be saved too?

int3 added a comment.Oct 18 2021, 10:02 AM

Chiming in late because I just got back from PTO...

Regarding the test: Yeah, we should avoid using yaml if possible. Maybe we could extend lc-linker-option.ll instead of creating a new test? We should also include some comments in the test explaining the motivation for running those particular test commands.

Also, does ASAN catch the use-after-free in this test? Figured it's worth checking to make sure that we are deterministically catching this error.

lld/MachO/Driver.cpp
116 ↗(On Diff #380307)

That seems really iffy; resolveDylibPath should return a safely-allocated StringRef regardless of how it's being called. Better to save() twice than to accidentally return an unsaved string.

Also, does ASAN catch the use-after-free in this test? Figured it's worth checking to make sure that we are deterministically catching this error.

It did (tried running the tests with ASAN enabled). - except we had no tests covering this path until now :(

Hello.

I rewrote my test using llvm IR, and add it to the lc-linker-option.ll test. I also add "saver.save()" wherever a function calls resolveDylibPath without saving the string itself, and removed saving StringRef instance in DriverUtils.cpp:line 199.

Shorten my test by removing some irrelevant symbols.

oontvoo accepted this revision as: oontvoo.Oct 19 2021, 8:03 AM

LGTM - thanks!
(leaving the lld-macho checkbox for int3, in case he has additional comments ... otherwise, I can check it EOD)

lld/test/MachO/lc-linker-option.ll
50–67

Include a comment here explaining what/why we're testing this (since this isn't testing linker option per se)?
(perhaps include reference to either the bug or this patch)

int3 added inline comments.Oct 19 2021, 12:12 PM
lld/MachO/Driver.cpp
116 ↗(On Diff #380307)

maybe you didn't see this comment :) I would prefer we revert to calling saver.save() within resolveDylibPath

int3 added inline comments.Oct 19 2021, 12:42 PM
lld/test/MachO/lc-linker-option.ll
128

this doesn't seem relevant?

I removed the unnecessary "Function Attrs" line in my test, and wrote some comments to explain why some lines (which are seemingly not testing linker options) is added to the test.

As for resolveDylibPath, I added a saver.save() inside this function to ensure it always returns a saved StringRef. Previously oontvoo says I might should not save a StringRef, which is assumed to be already saved when used, so I'm not very sure if I should save it here (I'm a beginner of llvm and not sure which is the right way to do it... ). Please contact me if this change needs further modification.

int3 accepted this revision.Oct 19 2021, 8:48 PM

Previously oontvoo says I might should not save a StringRef, which is assumed to be already saved when used

We can't always assume that. StringRef is basically just a nicer wrapper around char *. It just means that something else owns the string, but it may not necessarily by the StringSaver.

lld/MachO/Driver.cpp
116 ↗(On Diff #380307)

no need for saver.save() here any more

lld/MachO/InputFiles.cpp
906 ↗(On Diff #380842)

same, no need for save() here

918 ↗(On Diff #380842)

same

948 ↗(On Diff #380842)

same

lld/test/MachO/lc-linker-option.ll
51–57

thanks for the detailed comment!

This revision is now accepted and ready to land.Oct 19 2021, 8:48 PM
int3 added a comment.Oct 19 2021, 8:48 PM

Do you have commit permissions, or do you need us to land it for you?

PRESIDENT810 updated this revision to Diff 380854.EditedOct 19 2021, 9:30 PM

Removed some unnecessary saver.save().

I don't think I have permissions to commit this change, so could you please commit it for me? Thank you!

Removed some unnecessary saver.save().

I don't think I have permissions to commit this change, so could you please commit it for me? Thank you!

Done! Thanks!
(And sorry for the conflicting comments earlier)

This causes lc-linker-option.ll test failure on Windows:

$ ":" "RUN: at line 61"
$ "ln" "-sf" "A" "C:\b\s\w\ir\cache\builder\src\third_party\llvm-bootstrap\tools\lld\test\MachO\Output\lc-linker-option.ll.tmp/foo.framework/Versions/Current"
# command stderr:
'ln': command not found
error: command failed with exit status: 127
int3 added a comment.Oct 20 2021, 4:31 PM

Looking. Shouldn't be too hard to rewrite the test not to use frameworks

oontvoo added a comment.EditedOct 20 2021, 4:41 PM

Looking. Shouldn't be too hard to rewrite the test not to use frameworks

or maybe we could just cp rather than doing softlink?

P.S: i thought there were some windows buildbots... do they no longer run?

dyung added a subscriber: dyung.Oct 20 2021, 5:32 PM

Looking. Shouldn't be too hard to rewrite the test not to use frameworks

or maybe we could just cp rather than doing softlink?

P.S: i thought there were some windows buildbots... do they no longer run?

Our internal Windows build bot is also hitting an error on this change. We put the gnuwin32 tools on the path for it, and it appears that it only creates a shortcut since NTFS doesn't really support symbolic links. The linking command eventually fails like this:

$ "ld64.lld" "-arch" "x86_64" "-platform_version" "macos" "10.15" "11.0" "-syslibroot" "C:/Dev/git/merge/lld/test\MachO\Inputs\MacOSX.sdk" "-fatal_warnings" "-demangle" "-ObjC" "C:\Dev\git\merge\build-ps4-test\tools\lld\test\MachO\Output\lc-linker-option.ll.tmp/objfile1.o" "C:\Dev\git\merge\build-ps4-test\tools\lld\test\MachO\Output\lc-linker-option.ll.tmp/objfile2.o" "C:\Dev\git\merge\build-ps4-test\tools\lld\test\MachO\Output\lc-linker-option.ll.tmp/main.o" "-o" "C:\Dev\git\merge\build-ps4-test\tools\lld\test\MachO\Output\lc-linker-option.ll.tmp/main.out" "-arch" "x86_64" "-platform_version" "macos" "11.0.0" "0.0.0" "-FC:\Dev\git\merge\build-ps4-test\tools\lld\test\MachO\Output\lc-linker-option.ll.tmp"
# command stderr:
ld64.lld: error: framework not found for -framework foo
ld64.lld: error: framework not found for -framework foo

Maybe the cp approach would be better for Windows?

PRESIDENT810 updated this revision to Diff 381136.EditedOct 20 2021, 8:19 PM

Now my test uses cp command to create framework instead of ln command. Hope this will do, and if there is any problem, please let me know so I can try other workarounds.

I also noticed that in lld/test/MachO/framework.s, it also uses ln command to create frameworks (and requires shell to disable this test on Windows), see https://github.com/llvm/llvm-project/blob/6b715e9c4d9cc00f59906d48cd57f4c767229093/lld/test/MachO/framework.s#L7. If we can use cp to create frameworks and build successfully, maybe we can modify this test by replacing ln with cp, and enable this test on Windows? I'll be glad to submit another patch about this.

int3 added a comment.Oct 20 2021, 8:28 PM

I went down a rabbit hole while trying to fix this, discovering two different issues with our framework loading code in the process, which I've filed in https://bugs.llvm.org/show_bug.cgi?id=52246 and https://bugs.llvm.org/show_bug.cgi?id=52247. D112195: [lld-macho] Simplify lc-linker-option.ll and re-enable it on Windows is my follow-up diff.

As for framework.s, I would like us to keep it using symlinks, as I want to emulate a 'real' framework as closely as possible in at least one of our tests.

Please ignore my last updated diff, I didn't see int3's latest comment at that time. I will see what I could do about those two new issues, and as for this test, I think maybe I should make it conform to int3's newest revision (https://reviews.llvm.org/D112195).