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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: lld/MachO/Driver.cpp:116
 
-    if (Optional<std::string> path = resolveDylibPath(symlink))
+    if (Optional<StringRef> path = resolveDylibPath(symlink))
       return path;
----------------
int3 wrote:
> int3 wrote:
> > 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.
> maybe you didn't see this comment :) I would prefer we revert to calling `saver.save()` within `resolveDylibPath`
no need for saver.save() here any more


================
Comment at: lld/MachO/InputFiles.cpp:906
+        if (Optional<StringRef> dylibPath =
+                resolveDylibPath(saver.save(candidate.str())))
           return loadDylib(*dylibPath, umbrella);
----------------
same, no need for `save()` here


================
Comment at: lld/MachO/InputFiles.cpp:918
+      if (Optional<StringRef> dylibPath =
+              resolveDylibPath(saver.save((root + path).str())))
         return loadDylib(*dylibPath, umbrella);
----------------
same


================
Comment at: lld/MachO/InputFiles.cpp:948
+      if (Optional<StringRef> dylibPath =
+              resolveDylibPath(saver.save(newPath.str())))
         return loadDylib(*dylibPath, umbrella);
----------------
same


================
Comment at: lld/test/MachO/lc-linker-option.ll:51-57
+;; 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.
----------------
thanks for the detailed comment!


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

https://reviews.llvm.org/D111706