Use of macho.change_dylib means local builds generate invalid binaries

[fxcoudert]

brew gist-logs <formula> link OR brew config AND brew doctor output

HOMEBREW_VERSION: 3.5.4-95-gf041a59
ORIGIN: https://github.com/Homebrew/brew
HEAD: f041a59af9daddc7c7afe8537e3858103f8f9ab9
Last commit: 25 hours ago
Core tap ORIGIN: https://github.com/Homebrew/homebrew-core
Core tap HEAD: 31ce73e22aa47232cf3dafd497e54b734ae51186
Core tap last commit: 24 hours ago
Core tap branch: poppler
HOMEBREW_PREFIX: /opt/homebrew
HOMEBREW_CASK_OPTS: []
HOMEBREW_DISPLAY: /private/tmp/com.apple.launchd.n7vlxAKp1E/org.xquartz:0
HOMEBREW_EDITOR: vim
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_MAKE_JOBS: 8
HOMEBREW_NO_AUTO_UPDATE: set
Homebrew Ruby: 2.6.8 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: octa-core 64-bit arm_firestorm_icestorm
Clang: 13.1.6 build 1316
Git: 2.32.1 => /Applications/Xcode.app/Contents/Developer/usr/bin/git
Curl: 7.79.1 => /usr/bin/curl
macOS: 12.4-arm64
CLT: 13.4.0.0.1.1651278267
Xcode: 13.4.1
Rosetta 2: false

Verification

• [X] I ran brew update and am still able to reproduce my issue.
• [X] I have resolved all warnings from brew doctor and that did not fix my problem.

What were you trying to do (and why)?

Install poppler from source (brew install -svd poppler) then test it.

What happened (include all command output)?

$ /opt/homebrew/Cellar/poppler/22.06.0_1/bin/pdfinfo      
zsh: killed     /opt/homebrew/Cellar/poppler/22.06.0_1/bin/pdfinfo

What did you expect to happen?

Should not be killed

Step-by-step reproduction instructions (by running brew commands)

brew install -svd poppler && brew test -v poppler

[fxcoudert]

$ codesign -v -vvv /opt/homebrew/Cellar/poppler/22.06.0_1/bin/pdfinfo
/opt/homebrew/Cellar/poppler/22.06.0_1/bin/pdfinfo: invalid signature (code or signature have been modified)
In architecture: arm64

In the poppler formula we have this:

        macho = MachO.open(f)
        macho.change_dylib("@rpath/#{libpoppler}", "#{opt_lib}/#{libpoppler}")
        macho.write!

but because the files are not resigned, they're now invalid binaries. This does not show on bottles, because when pouring bottles we resign everything. But it shows when building stuff locally.

Three formulas use that:

$ grep macho.change_dylib *
netcdf.rb:      macho.change_dylib("@rpath/#{libnetcdf}", "#{lib}/#{libnetcdf}")
poppler-qt5.rb:        macho.change_dylib("@rpath/#{libpoppler}", "#{opt_lib}/#{libpoppler}")
poppler.rb:        macho.change_dylib("@rpath/#{libpoppler}", "#{opt_lib}/#{libpoppler}")

[carlocab]

Needs to be codesigned after doing that. But we really don't need to call change_dylib; we can just set CMAKE_INSTALL_RPATH instead.

diff --git a/Formula/poppler.rb b/Formula/poppler.rb
index e610f7e24ef..a4bb5e5ff4b 100644
--- a/Formula/poppler.rb
+++ b/Formula/poppler.rb
@@ -69,7 +69,7 @@ class Poppler < Formula
       -DWITH_GObjectIntrospection=ON
     ]

-    system "cmake", ".", *args
+    system "cmake", ".", *args, "-DCMAKE_INSTALL_RPATH=#{rpath}"
     system "make", "install"
     system "make", "clean"
     system "cmake", ".", "-DBUILD_SHARED_LIBS=OFF", *args
@@ -80,20 +80,6 @@ class Poppler < Formula
     resource("font-data").stage do
       system "make", "install", "prefix=#{prefix}"
     end
-
-    if OS.mac?
-      libpoppler = (lib/"libpoppler.dylib").readlink
-      [
-        "#{lib}/libpoppler-cpp.dylib",
-        "#{lib}/libpoppler-glib.dylib",
-        "#{lib}/libpoppler-qt#{Formula["qt"].version.major}.dylib",
-        *Dir["#{bin}/*"],
-      ].each do |f|
-        macho = MachO.open(f)
-        macho.change_dylib("@rpath/#{libpoppler}", "#{opt_lib}/#{libpoppler}")
-        macho.write!
-      end
-    end
   end

   test do

[fxcoudert]

Yes, testing that fix in https://github.com/Homebrew/homebrew-core/pull/105964#issuecomment-1186380022

[fxcoudert]

More cases:

ampl-mp.rb:      MachO::Tools.change_install_name("bin/libasl.dylib", "@rpath/libmp.3.dylib",
boost-mpi.rb:      MachO::Tools.change_install_name("#{lib}/libboost_mpi-mt.dylib",
boost-mpi.rb:      MachO::Tools.change_install_name("#{lib}/libboost_mpi.dylib",
espeak.rb:      MachO::Tools.change_dylib_id("#{lib}/libespeak.dylib", "#{lib}/libespeak.dylib") if OS.mac?
grpc.rb:        MachO::Tools.add_rpath(bin/"grpc_cli", rpath)
grpc.rb:        MachO::Tools.add_rpath(lib/shared_library("libgrpc++_test_config"), rpath)
libsvm.rb:    MachO::Tools.change_dylib_id("#{lib}/libsvm.2.dylib", "#{lib}/libsvm.2.dylib") if OS.mac?
plplot.rb:          MachO::Tools.change_install_name(f, d, d_new)
pypy.rb:      MachO::Tools.change_install_name("#{libexec}/bin/pypy",
pypy3.rb:      MachO::Tools.change_install_name("#{libexec}/bin/pypy3",
pypy3.rb:      MachO::Tools.change_dylib_id("#{libexec}/lib/libpypy3-c.dylib",
rust.rb:      MachO::Tools.change_dylib_id(dylib, "@rpath/#{File.basename(dylib)}")

[MikeMcQuaid]

This seems sufficiently fixed.