[lld] Do not implicitly link non "public" libraries

[drodriguez]

The LC_SUB_CLIENT Mach-O command and the allowable-clients TBD entry specify that the given framework (or library?) can only be linked directly from the specified names, even if it is sitting in /usr/lib or /System/Library/Frameworks.

Add a check for those conditions before checking if a library should be implicitly linked, and link against their umbrella if they have allowable clients. The code needs to be in both the binary libraries and the interface libraries.

Add a test that reproduces the scenario in which a framework reexports a private framework that sits in /System/Library/Frameworks, and check for the symbols of the reexported framework to be associated with the public framework, and not the private one.

[llvmbot]

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-macho

Author: Daniel Rodríguez Troitiño (drodriguez)

Changes

The LC_SUB_CLIENT Mach-O command and the allowable-clients TBD entry specify that the given framework (or library?) can only be linked directly from the specified names, even if it is sitting in /usr/lib or /System/Library/Frameworks.

Add a check for those conditions before checking if a library should be implicitly linked, and link against their umbrella if they have allowable clients. The code needs to be in both the binary libraries and the interface libraries.

Add a test that reproduces the scenario in which a framework reexports a private framework that sits in /System/Library/Frameworks, and check for the symbols of the reexported framework to be associated with the public framework, and not the private one.

---

Full diff: https://github.com/llvm/llvm-project/pull/97639.diff

2 Files Affected:

• (modified) lld/MachO/InputFiles.cpp (+8-2)
• (added) lld/test/MachO/implicit-and-allowable-clients.test (+49)

diff --git a/lld/MachO/InputFiles.cpp b/lld/MachO/InputFiles.cpp
index 0cee1a84d0b55..b40a812f30bd3 100644
--- a/lld/MachO/InputFiles.cpp
+++ b/lld/MachO/InputFiles.cpp
@@ -1725,7 +1725,10 @@ DylibFile::DylibFile(MemoryBufferRef mb, DylibFile *umbrella,
   }
 
   // Initialize symbols.
-  exportingFile = isImplicitlyLinked(installName) ? this : this->umbrella;
+  bool canBeImplicitlyLinked = findCommand(hdr, LC_SUB_CLIENT) == nullptr;
+  exportingFile = (canBeImplicitlyLinked && isImplicitlyLinked(installName))
+                      ? this
+                      : this->umbrella;
 
   const auto *dyldInfo = findCommand<dyld_info_command>(hdr, LC_DYLD_INFO_ONLY);
   const auto *exportsTrie =
@@ -1884,7 +1887,10 @@ DylibFile::DylibFile(const InterfaceFile &interface, DylibFile *umbrella,
 
   checkAppExtensionSafety(interface.isApplicationExtensionSafe());
 
-  exportingFile = isImplicitlyLinked(installName) ? this : umbrella;
+  bool canBeImplicitlyLinked = interface.allowableClients().size() == 0;
+  exportingFile = (canBeImplicitlyLinked && isImplicitlyLinked(installName))
+                      ? this
+                      : umbrella;
   auto addSymbol = [&](const llvm::MachO::Symbol &symbol,
                        const Twine &name) -> void {
     StringRef savedName = saver().save(name);
diff --git a/lld/test/MachO/implicit-and-allowable-clients.test b/lld/test/MachO/implicit-and-allowable-clients.test
new file mode 100644
index 0000000000000..97123ab9c5dce
--- /dev/null
+++ b/lld/test/MachO/implicit-and-allowable-clients.test
@@ -0,0 +1,49 @@
+# REQUIRES: arm
+# RUN: rm -rf %t; split-file %s %t
+# RUN: ln -s Versions/A/Framework1.tbd %t/System/Library/Frameworks/Framework1.framework/
+# RUN: ln -s Versions/A/Framework11.tbd %t/System/Library/Frameworks/Framework11.framework/
+# RUN: llvm-mc -filetype obj -triple arm64-apple-macos11.0 %t/test.s -o %t/test.o
+# RUN: %lld -arch arm64 -platform_version macos 11.0 11.0 -o %t/test -syslibroot %t -framework Framework1 %t/test.o
+
+# RUN: llvm-objdump --bind --no-show-raw-insn -d %t/test | FileCheck %s
+# CHECK:     Bind table:
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 Framework1   _func1
+# CHECK-DAG: __DATA __data {{.*}} pointer 0 Framework1   _func11
+
+#--- System/Library/Frameworks/Framework1.framework/Versions/A/Framework1.tbd
+--- !tapi-tbd
+tbd-version: 4
+targets: [ arm64-macos ]
+install-name: '/System/Library/Frameworks/Framework1.framework/Versions/A/Framework1'
+current-version: 1.0.0
+reexported-libraries:
+  - targets: [ arm64-macos ]
+    libraries: [ '/System/Library/Frameworks/Framework11.framework/Versions/A/Framework11' ]
+exports:
+  - targets: [ arm64-macos ]
+    symbols: [ '_func1' ]
+...
+#--- System/Library/Frameworks/Framework11.framework/Versions/A/Framework11.tbd
+--- !tapi-tbd
+tbd-version: 4
+targets: [ arm64-macos ]
+install-name: '/System/Library/Frameworks/Framework11.framework/Versions/A/Framework11'
+current-version: 1.0.0
+allowable-clients:
+  - targets: [ arm64-macos ]
+    clients: [ Framework1 ]
+exports:
+  - targets: [ arm64-macos ]
+    symbols: [ '_func11' ]
+...
+#--- test.s
+.text
+.globl _main
+
+_main:
+  ret
+
+.data
+  .quad _func1
+  .quad _func11
+

[tstellar]

You have a typo in the PR summary:

s/ldd/lld/

[dyung]

Was this change approved before it was submitted?

[bdash]

Apple's linker rejects linking to libraries with allowable client lists:

$ cc -framework vecLib -o test test.m         
ld: cannot link directly with 'vecLib' because product being built is not an allowed client of it
clang: error: linker command failed with exit code 1 (use -v to see invocation)

After this change, not only does lld not reject linking to vecLib.framework, it links to the library despite the binary not being an allowed client:

$ cc -framework vecLib -fuse-ld=lld -Bllvm-build/Release+Asserts/bin -o test test.m
$ nm -m test | grep vecLib                                                                                               
                 (undefined) external _vvdiv (from vecLib)
$ grep ' clients:' $(xcrun --show-sdk-path)/System/Library/Frameworks/Accelerate.framework/Versions/Current/Frameworks/vecLib.framework/vecLib.tbd
    clients:         [ Accelerate ]
    clients:         [ vecLib ]
    clients:         [ vecLib ]
    clients:         [ vecLib ]
    clients:         [ vecLib ]
    clients:         [ Sparse, vecLib ]

What was the motivation for this change? Generating binaries that don't respect the allowable client list means applications will break if / when Apple moves symbols between libraries. Is this intentional?

There's no bug associated with this change so I cannot tell what problem it is trying to solve.

test.m:

#include <stdio.h>
#include <vecLib/vForce.h>

int main(int argc, char**argv) {
    fprintf(stderr, "%p\n", &vvdiv);
    return 0;
}

[drodriguez]

What was the motivation for this change?

The motivation is more or less replicated on the test. With the Xcode 16 SDKs there's a new "private" framework that does not exist in previous SDKs, but it was reexported from the "public" framework that existed in previous versions. The code in isImplicitlyLinked used to consider the libraries in /usr/lib and /System/Library/Frameworks linkable. The new "private" framework lives in one of those paths, so it was a candidate to be implicitly linked, with the bad luck that previous versions of the OS will not find the new file and fail to start the binaries. With the new code, libraries are only implicitly linked if the libraries do not have "allowable clients", which is the case in the new SDKs. Libraries with "allowable clients" should had never been linked directly, anyway.

After this change, not only does lld not reject linking to vecLib.framework, it links to the library despite the binary not being an allowed client:

This was true before and after this change. LLD does not honor "allowable clients" at all. If you explicitly link against a private library, LLD will do what you ask. The changes above was trying to avoid a implicit link, but it was not trying to avoid any explicit link the user tries to do in the command line. I think nobody has taken the time to implement all the system of alloweable clients into LLD. It is of limited value to anybody that do not develop SDKs, which in the case of macOS, it is just Apple, and they use their own linker.

What this change was doing was that when linking to Accelerate (an allowable client of vecLib), vecLib will not be linked directly, but Accelerate will. Before this change, depending on the symbols, vecLib might have been linked incorrectly.

[bdash]

Thanks for the explanation. Shortly before you posted your comment I found I had blamed this change in error and that, as you note, this change did not change the behavior I had observed. My apologies for that error.

It is of limited value to anybody that do not develop SDKs, which in the case of macOS, it is just Apple, and they use their own linker.

There's a huge benefit to validating allowable clients for third-party developers that use Apple's SDKs. Apple uses the allowed clients list to limit what is considered part of the public ABI that they have to support. If you unintentionally link directly to a framework that restricts its allowable clients, your software will break in a software update if Apple moves symbols around. Apple uses their symbol versioning tricks to preserve binary compatibility when moving public symbols, but they're unlikely to consider that for libraries that they consider to be private. If the linker does not catch this class of mistake you can end up shipping a time bomb.

[drodriguez]

My apologies. The phrase about being only useful to Apple was more about being able to link libraries restricted like that, but you are right that the restriction for linking against those libraries are still useful for everyone. We catch this problem early and avoided major issues, but it can be really bad if there's other "holes" in the implementation. Hopefully the issue you opened will catch the eye of someone.

[carlocab]

Here's an implementation of restricting linking to libraries linked with -allowable_client: #114638

Please have a look if you have the time.