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

Mon Oct 18 10:02:40 PDT 2021

int3 added a comment.

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.

================
Comment at: lld/MachO/Driver.cpp:116

-    if (Optional<std::string> path = resolveDylibPath(symlink))
+    if (Optional<StringRef> path = resolveDylibPath(symlink))
       return path;
----------------
oontvoo wrote:
> 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.
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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111706/new/

https://reviews.llvm.org/D111706