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

[PRESIDENT810]

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.

[oontvoo]

.... 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?

[PRESIDENT810]

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

[oontvoo]

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]

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]

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]

[clearing queue]

[PRESIDENT810]

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.

[oontvoo]

In D111706#3069533, @PRESIDENT810 wrote:

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]

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.

[oontvoo]

In D111706#3070818, @int3 wrote:

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 :(

[PRESIDENT810]

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.

[PRESIDENT810]

Shorten my test by removing some irrelevant symbols.

[oontvoo]

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]

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]

lld/test/MachO/lc-linker-option.ll

128

this doesn't seem relevant?

[PRESIDENT810]

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]

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!

[int3]

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

[PRESIDENT810]

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!

[oontvoo]

In D111706#3074387, @PRESIDENT810 wrote:

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)

[zequanwu]

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]

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

[oontvoo]

In D111706#3076692, @int3 wrote:

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]

In D111706#3076699, @oontvoo wrote:

In D111706#3076692, @int3 wrote:

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]

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]

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.

[PRESIDENT810]

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).

lld/test/MachO/lc-linker-option.ll

-; REQUIRES: x86, shell
+; REQUIRES: x86
 ; RUN: rm -rf %t; split-file %s %t
 
 ; RUN: llvm-as %t/framework.ll -o %t/framework.o
 ; RUN: %lld -lSystem %t/framework.o -o %t/frame
 ; RUN: llvm-otool -l %t/frame | FileCheck --check-prefix=FRAME %s \
 ; RUN:  --implicit-check-not LC_LOAD_DYLIB
 ; FRAME:          cmd LC_LOAD_DYLIB
 ; FRAME-NEXT: cmdsize

 ; LIB2-NEXT:    name /System/Library/Frameworks/CoreFoundation.framework/CoreFoundation
 ; LIB2:          cmd LC_LOAD_DYLIB
 ; LIB2-NEXT: cmdsize
 ; LIB2-NEXT:    name /usr/lib/libc++abi.dylib
 
 ; RUN: llvm-as %t/invalid.ll -o %t/invalid.o
 ; RUN: not %lld %t/invalid.o -o /dev/null 2>&1 | FileCheck --check-prefix=INVALID %s
 ; INVALID: error: -why_load is not allowed in LC_LINKER_OPTION
 
 ;; We are testing this because we want to check a dangling string reference problem (see https://reviews.llvm.org/D111706).
 ;; To trigger this problem, we need to create a framework that is an archive,
 ;; and it needs to contain a symbol starting with OBJC_CLASS_$.
 ;; The bug is triggered as the linker loads this framework twice via LC_LINKER_OPTION.
 ;; When the linker adds this framework, it will fail to map the path of framework to this archive due to dangling reference.
 ;; Therefore, it will load the framework twice, and if there is any symbol with OBJC_CLASS_$ prefix with forceLoadObjC enabled,
 ;; the linker will fetch this symbol twice, which leads to a duplicate symbol error.
 ; RUN: llc %t/foo.ll -o %t/foo.o -filetype=obj
 ; RUN: mkdir -p %t/foo.framework/Versions/A
 ; RUN: llvm-ar rcs %t/foo.framework/Versions/A/foo %t/foo.o
-; RUN: ln -sf A %t/foo.framework/Versions/Current
-; RUN: ln -sf Versions/Current/foo %t/foo.framework/foo
+; RUN: cp %t/foo.framework/Versions/A/foo %t/foo.framework/foo
+; RUN: cp -r %t/foo.framework/Versions/A/ %t/foo.framework/Versions/Current/
 ; RUN: llc %t/objfile1.ll -o %t/objfile1.o -filetype=obj
 ; RUN: llc %t/objfile2.ll -o %t/objfile2.o -filetype=obj
 ; RUN: llc %t/main.ll -o %t/main.o -filetype=obj
 ; RUN: %lld -demangle -ObjC %t/objfile1.o %t/objfile2.o %t/main.o -o %t/main.out -arch x86_64 -platform_version macos 11.0.0 0.0.0 -F%t
 
 ;--- framework.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"CoreFoundation"}
 !llvm.linker.options = !{!0}
 
 declare void @_CFBigNumGetInt128(...)

 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 !0 = !{!"-framework", !"foo"}
 !llvm.linker.options = !{!0}
 
 ;--- main.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 define void @main() {
   ret void
 }
 
 ;--- foo.ll
 target triple = "x86_64-apple-macosx10.15.0"
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 
 %0 = type opaque
 %struct._class_t = type {}
 
 @"OBJC_CLASS_$_TestClass" = global %struct._class_t {}, section "__DATA, __objc_data", align 8