[lld] aab0f22 - [lld-macho] Fix dangling string reference when adding frameworks
Vy Nguyen via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 20 08:22:22 PDT 2021
Author: Kaining Zhong
Date: 2021-10-20T11:21:40-04:00
New Revision: aab0f2264aebe6cb89d5dcb30df664bfc4d8f1c3
URL: https://github.com/llvm/llvm-project/commit/aab0f2264aebe6cb89d5dcb30df664bfc4d8f1c3
DIFF: https://github.com/llvm/llvm-project/commit/aab0f2264aebe6cb89d5dcb30df664bfc4d8f1c3.diff
LOG: [lld-macho] Fix dangling string reference when adding frameworks
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.
Reviewed By: int3, #lld-macho, oontvoo
Differential Revision: https://reviews.llvm.org/D111706
Added:
Modified:
lld/MachO/Driver.cpp
lld/MachO/Driver.h
lld/MachO/DriverUtils.cpp
lld/MachO/InputFiles.cpp
lld/test/MachO/lc-linker-option.ll
Removed:
################################################################################
diff --git a/lld/MachO/Driver.cpp b/lld/MachO/Driver.cpp
index 0611f556fa526..527e027381be6 100644
--- a/lld/MachO/Driver.cpp
+++ b/lld/MachO/Driver.cpp
@@ -92,7 +92,7 @@ static Optional<StringRef> findLibrary(StringRef name) {
{".tbd", ".dylib", ".a"});
}
-static Optional<std::string> findFramework(StringRef name) {
+static Optional<StringRef> findFramework(StringRef name) {
SmallString<260> symlink;
StringRef suffix;
std::tie(name, suffix) = name.split(",");
@@ -108,12 +108,12 @@ static Optional<std::string> findFramework(StringRef name) {
// only append suffix if realpath() succeeds
Twine suffixed = location + suffix;
if (fs::exists(suffixed))
- return suffixed.str();
+ return saver.save(suffixed.str());
}
// Suffix lookup failed, fall through to the no-suffix case.
}
- if (Optional<std::string> path = resolveDylibPath(symlink))
+ if (Optional<StringRef> path = resolveDylibPath(symlink.str()))
return path;
}
return {};
@@ -351,7 +351,7 @@ static void addLibrary(StringRef name, bool isNeeded, bool isWeak,
static void addFramework(StringRef name, bool isNeeded, bool isWeak,
bool isReexport, bool isExplicit) {
- if (Optional<std::string> path = findFramework(name)) {
+ if (Optional<StringRef> path = findFramework(name)) {
if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
addFile(*path, /*forceLoadArchive=*/false, isExplicit))) {
if (isNeeded)
diff --git a/lld/MachO/Driver.h b/lld/MachO/Driver.h
index b2d5721290e7e..af0b1d3e1f440 100644
--- a/lld/MachO/Driver.h
+++ b/lld/MachO/Driver.h
@@ -52,7 +52,7 @@ void parseLCLinkerOption(InputFile *, unsigned argc, StringRef data);
std::string createResponseFile(const llvm::opt::InputArgList &args);
// Check for both libfoo.dylib and libfoo.tbd (in that order).
-llvm::Optional<std::string> resolveDylibPath(llvm::StringRef path);
+llvm::Optional<StringRef> resolveDylibPath(llvm::StringRef path);
DylibFile *loadDylib(llvm::MemoryBufferRef mbref, DylibFile *umbrella = nullptr,
bool isBundleLoader = false);
diff --git a/lld/MachO/DriverUtils.cpp b/lld/MachO/DriverUtils.cpp
index cbbb242aed755..a38d4ab075209 100644
--- a/lld/MachO/DriverUtils.cpp
+++ b/lld/MachO/DriverUtils.cpp
@@ -183,7 +183,7 @@ static void searchedDylib(const Twine &path, bool found) {
depTracker->logFileNotFound(path);
}
-Optional<std::string> macho::resolveDylibPath(StringRef dylibPath) {
+Optional<StringRef> macho::resolveDylibPath(StringRef dylibPath) {
// TODO: if a tbd and dylib are both present, we should check to make sure
// they are consistent.
SmallString<261> tbdPath = dylibPath;
@@ -191,12 +191,12 @@ Optional<std::string> macho::resolveDylibPath(StringRef dylibPath) {
bool tbdExists = fs::exists(tbdPath);
searchedDylib(tbdPath, tbdExists);
if (tbdExists)
- return std::string(tbdPath);
+ return saver.save(tbdPath.str());
bool dylibExists = fs::exists(dylibPath);
searchedDylib(dylibPath, dylibExists);
if (dylibExists)
- return std::string(dylibPath);
+ return saver.save(dylibPath);
return {};
}
diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index 008746f53b38b..65abaf7b3d988 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -902,7 +902,7 @@ static DylibFile *findDylib(StringRef path, DylibFile *umbrella,
for (StringRef dir : config->frameworkSearchPaths) {
SmallString<128> candidate = dir;
path::append(candidate, frameworkName);
- if (Optional<std::string> dylibPath = resolveDylibPath(candidate))
+ if (Optional<StringRef> dylibPath = resolveDylibPath(candidate.str()))
return loadDylib(*dylibPath, umbrella);
}
} else if (Optional<StringRef> dylibPath = findPathCombination(
@@ -913,8 +913,7 @@ static DylibFile *findDylib(StringRef path, DylibFile *umbrella,
// 2. As absolute path.
if (path::is_absolute(path, path::Style::posix))
for (StringRef root : config->systemLibraryRoots)
- if (Optional<std::string> dylibPath =
- resolveDylibPath((root + path).str()))
+ if (Optional<StringRef> dylibPath = resolveDylibPath((root + path).str()))
return loadDylib(*dylibPath, umbrella);
// 3. As relative path.
@@ -943,7 +942,7 @@ static DylibFile *findDylib(StringRef path, DylibFile *umbrella,
path::remove_filename(newPath);
}
path::append(newPath, rpath, path.drop_front(strlen("@rpath/")));
- if (Optional<std::string> dylibPath = resolveDylibPath(newPath))
+ if (Optional<StringRef> dylibPath = resolveDylibPath(newPath.str()))
return loadDylib(*dylibPath, umbrella);
}
}
@@ -961,7 +960,7 @@ static DylibFile *findDylib(StringRef path, DylibFile *umbrella,
}
}
- if (Optional<std::string> dylibPath = resolveDylibPath(path))
+ if (Optional<StringRef> dylibPath = resolveDylibPath(path))
return loadDylib(*dylibPath, umbrella);
return nullptr;
diff --git a/lld/test/MachO/lc-linker-option.ll b/lld/test/MachO/lc-linker-option.ll
index e80ccf6ade767..12910a364a8a8 100644
--- a/lld/test/MachO/lc-linker-option.ll
+++ b/lld/test/MachO/lc-linker-option.ll
@@ -48,6 +48,23 @@
; 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: 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"
@@ -90,3 +107,34 @@ target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16
define void @main() {
ret void
}
+
+;--- objfile1.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", !"foo"}
+!llvm.linker.options = !{!0}
+
+;--- objfile2.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", !"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
\ No newline at end of file
More information about the llvm-commits
mailing list