mac/formula_cellar_checks: check for flat namespace libraries #12225

carlocab · 2021-10-12T05:29:49Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

There are at least five instances where a formula has libraries compiled
with -flat_namespace due to a bug in detecting the macOS version (cf.
Homebrew/homebrew-core#87103, Homebrew/homebrew-core#85974,
Homebrew/homebrew-core#85973).

I think it makes sense to check for this more generally. It is
sometimes intentional, so I've added a check for an allowlist for
those instances. Running this on the current util-linux bottle
produces

❯ brew audit --strict util-linux
util-linux:
  * Libraries were compiled with a flat namespace.
    This can cause linker errors due to name collisions, and
    is often due to a bug in detecting the macOS version.
      /usr/local/Cellar/util-linux/2.37.2/lib/libblkid.1.dylib
      /usr/local/Cellar/util-linux/2.37.2/lib/libfdisk.1.dylib
      /usr/local/Cellar/util-linux/2.37.2/lib/libsmartcols.1.dylib
      /usr/local/Cellar/util-linux/2.37.2/lib/libuuid.1.dylib
Error: 1 problem in 1 formula detected

Some things that still need to be done here:

~~fix this check for universal binaries~~
check if we want to restrict this audit to newer versions of macOS
~~fix false positives (try brew audit --strict llvm and compare the~~
~~output of otool -hV on the identified files)~~

While we're here, let's fix the formatting of the output of these other
audits (cf. #12217).

Here's some background on flat vs two-level namespaces: https://developer.apple.com/library/archive/documentation/Porting/Conceptual/PortingUnix/compiling/compiling.html#//apple_ref/doc/uid/TP40002850-BCIHJBBF

There are at least five instances where a formula has libraries compiled with `-flat_namespace` due to a bug in detecting the macOS version (cf. Homebrew/homebrew-core#87103, Homebrew/homebrew-core#85974, Homebrew/homebrew-core#85973). I think it makes sense to check for this more generally. It is sometimes intentional, so I've added a check for an allowlist for those instances. Running this on the current `util-linux` bottle produces ❯ brew audit --strict util-linux util-linux: * Libraries were compiled with a flat namespace. This can cause linker errors due to name collisions, and is often due to a bug in detecting the macOS version. /usr/local/Cellar/util-linux/2.37.2/lib/libblkid.1.dylib /usr/local/Cellar/util-linux/2.37.2/lib/libfdisk.1.dylib /usr/local/Cellar/util-linux/2.37.2/lib/libsmartcols.1.dylib /usr/local/Cellar/util-linux/2.37.2/lib/libuuid.1.dylib Error: 1 problem in 1 formula detected Some things that still need to be done here: - fix this check for universal binaries - check if we want to restrict this audit check to newer versions of macOS - fix false positives (try `brew audit --strict llvm` and compare the output of `otool -hV` on the identified files) While we're here, let's fix the formatting of the output of these other audits (cf. #12217).

BrewTestBot · 2021-10-12T05:30:04Z

Review period will end on 2021-10-13 at 05:29:49 UTC.

carlocab · 2021-10-12T06:32:32Z

~~This still produces a bunch of false positives, which seems like a bug in ruby-macho. Will have to fix that.~~ Fixed. It was a typo. 😅

There was a typo that made it so that all libraries were being included in `flat_namespace_files`.

MikeMcQuaid

LGTM, nice work!

carlocab · 2021-10-12T12:02:28Z

For reference, here is a list of formulae I have installed on Big Sur whose libraries have been compiled with -flat_namespace:

berkeley-db
gdbm
gettext
gmp
gpgme
gsl
gts
isl
jq
julia
libassuan
libb2
libev
libksba
libmpc
mpfr
npth
openldap
pcre
pcre2
tcl-tk

Of these, I believe only gts, julia, and tcl-tk are intentional. I concluded this from checking the Catalina bottles whether they were also compiled with -flat_namespace.

carlocab · 2021-10-12T12:07:35Z

Oh, here's another to-do. This check doesn't work for weird libraries like this one:

❯ file $(brew --prefix gcc)/lib/gcc/11/libgcc_s.1.dylib
/usr/local/opt/gcc/lib/gcc/11/libgcc_s.1.dylib: Mach-O universal binary with 1 architecture: [x86_64:Mach-O 64-bit dynamically linked shared library x86_64]
/usr/local/opt/gcc/lib/gcc/11/libgcc_s.1.dylib (for architecture x86_64):	Mach-O 64-bit dynamically linked shared library x86_64

The issue is that #universal? returns false (it only contains one slice), but ruby-macho parses this as a FatFile (based on the MachO header), and FatFiles have no header method.

I think I just need to change the if file.universal? check to a case macho.class.

Library/Homebrew/extend/os/mac/formula_cellar_checks.rb

Bo98 · 2021-10-12T12:22:43Z

My run on Mojave:

mono:
  * Libraries were compiled with a flat namespace.
    This can cause linker errors due to name collisions, and
    is often due to a bug in detecting the macOS version.
      /usr/local/Cellar/mono/6.12.0.122/lib/libmono-profiler-aot.0.dylib
      /usr/local/Cellar/mono/6.12.0.122/lib/libmono-profiler-coverage.0.dylib
      /usr/local/Cellar/mono/6.12.0.122/lib/libmono-profiler-log.0.dylib
ocaml:
  * Libraries were compiled with a flat namespace.
    This can cause linker errors due to name collisions, and
    is often due to a bug in detecting the macOS version.
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/bigarray.cmxs
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/libasmrun_shared.so
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/libcamlrun_shared.so
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/str.cmxs
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/stublibs/dllcamlstr.so
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/stublibs/dllthreads.so
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/stublibs/dllunix.so
      /usr/local/Cellar/ocaml/4.12.0/lib/ocaml/unix.cmxs
ocaml-num:
  * Libraries were compiled with a flat namespace.
    This can cause linker errors due to name collisions, and
    is often due to a bug in detecting the macOS version.
      /usr/local/Cellar/ocaml-num/1.4_2/lib/ocaml/nums.cmxs
      /usr/local/Cellar/ocaml-num/1.4_2/lib/ocaml/stublibs/dllnums.so
tcl-tk:
  * Libraries were compiled with a flat namespace.
    This can cause linker errors due to name collisions, and
    is often due to a bug in detecting the macOS version.
      /usr/local/Cellar/tcl-tk/8.6.11_1/lib/tcltls1.7.22/tcltls.dylib
xvid:
  * Libraries were compiled with a flat namespace.
    This can cause linker errors due to name collisions, and
    is often due to a bug in detecting the macOS version.
      /usr/local/Cellar/xvid/1.3.7/lib/libxvidcore.4.dylib

I'm a little concerned at how many exceptions there is going to be. Though equally, one can wonder why some of these need -flat_namespace. For example, the flag was added to xvid for PowerPC in 2003.

MikeMcQuaid · 2021-10-12T12:28:45Z

Though equally, one can wonder why some of these need -flat_namespace. For example, the flag was added to xvid for PowerPC in 2003.

This is the type of thing I wonder if we just strip out with superenv (and allow exceptions).

carlocab · 2021-10-12T12:30:44Z

I'm a little concerned at how many exceptions there is going to be.

I'm concerned about this too. I'm not sure how to handle it though.

Though equally, one can wonder why some of these need -flat_namespace. For example, the flag was added to xvid for PowerPC in 2003.

Agreed. My view is that flat namespaces are good for development and debugging (but then you can use DYLD_FORCE_FLAT_NAMESPACE). Actually using them in production seems like a bit of an anti-pattern to me, especially given that I don't think you can rely on the linker's behaviour regarding how it chooses to resolve collisions.

Bo98 · 2021-10-12T12:43:34Z

This is the type of thing I wonder if we just strip out with superenv (and allow exceptions).

Could do. I'd like to keep superenv exceptions to even more of a minimum as it indicates we are incorrectly stripping something. If it's just one or two that need it then it's probably ok.

Library/Homebrew/extend/os/mac/formula_cellar_checks.rb

MikeMcQuaid · 2021-10-12T12:57:46Z

Could do. I'd like to keep superenv exceptions to even more of a minimum as it indicates we are incorrectly stripping something. If it's just one or two that need it then it's probably ok.

Oh, agreed, this just seems like the type of thing where Superenv stripping is going to be easier than patching all these build systems.

FnControlOption · 2021-10-12T17:36:39Z

I made a patch that emulates the libtool fix for this by removing -flat_namespace and replacing -undefined suppress with -undefined dynamic_lookup:

diff --git a/Library/Homebrew/shims/super/cc b/Library/Homebrew/shims/super/cc
index c629efa9e..d0f731830 100755
--- a/Library/Homebrew/shims/super/cc
+++ b/Library/Homebrew/shims/super/cc
@@ -208,6 +208,12 @@ class Cmd
       args << arg unless tool =~ /^g..-4.[02]/
     when /^-Wl,-z,defs/
       # -Wl,-undefined,error is already the default
+    when "-Wl,-flat_namespace"
+      # remove arg
+    when "-Wl,-undefined"
+      arg2 = enum.next
+      arg2 = "-Wl,dynamic_lookup" if arg2 == "-Wl,suppress"
+      args << arg << arg2
     when /^-W[alp],/, /^-Wno-/, "-Werror=implicit-function-declaration"
       args << arg
     when /^-W.*/

BrewTestBot · 2021-10-13T06:01:54Z

Review period ended.

Library/Homebrew/extend/os/mac/formula_cellar_checks.rb

- Fix missing space - use `MachO::Utils.fat_magic?` - call `#flag?` consistently. Co-authored-by: Bo Anderson <mail@boanderson.me>

carlocab · 2021-10-18T14:58:54Z

Let's try this. We can revert and/or strip the -flat_namespace flag if this gets too annoying.

@Homebrew/core, if you see audit failures with the text

       Libraries were compiled with a flat namespace.
       This can cause linker errors due to name collisions, and
       is often due to a bug in detecting the macOS version.

feel free to ping/assign me.

BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Oct 12, 2021

carlocab mentioned this pull request Oct 12, 2021

util-linux: fix libtool.m4 for macOS 11+ Homebrew/homebrew-core#87103

Closed

6 tasks

Fix universal binary handling in check_flat_namespace

7223f8e

Fix false positives in audit

4fbe0a2

There was a typo that made it so that all libraries were being included in `flat_namespace_files`.

MikeMcQuaid approved these changes Oct 12, 2021

View reviewed changes

Bo98 reviewed Oct 12, 2021

View reviewed changes

Library/Homebrew/extend/os/mac/formula_cellar_checks.rb Outdated Show resolved Hide resolved

Library/Homebrew/extend/os/mac/formula_cellar_checks.rb Show resolved Hide resolved

carlocab commented Oct 12, 2021

View reviewed changes

Library/Homebrew/extend/os/mac/formula_cellar_checks.rb Outdated Show resolved Hide resolved

BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Oct 12, 2021

BrewTestBot approved these changes Oct 13, 2021

View reviewed changes

carlocab commented Oct 13, 2021

View reviewed changes

Library/Homebrew/extend/os/mac/formula_cellar_checks.rb Outdated Show resolved Hide resolved

mac/formula_cellar_checks: apply suggestions from code review

e8eb781

- Fix missing space - use `MachO::Utils.fat_magic?` - call `#flag?` consistently. Co-authored-by: Bo Anderson <mail@boanderson.me>

carlocab merged commit 556b8f1 into Homebrew:master Oct 18, 2021

carlocab deleted the two-level branch October 18, 2021 14:59

github-actions bot added the outdated PR was locked due to age label Nov 18, 2021

github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2021

Uh oh!

mac/formula_cellar_checks: check for flat namespace libraries #12225

mac/formula_cellar_checks: check for flat namespace libraries #12225

Uh oh!

Conversation

carlocab commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrewTestBot commented Oct 12, 2021

Uh oh!

carlocab commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Uh oh!

carlocab commented Oct 12, 2021

Uh oh!

carlocab commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Bo98 commented Oct 12, 2021

Uh oh!

MikeMcQuaid commented Oct 12, 2021

Uh oh!

carlocab commented Oct 12, 2021

Uh oh!

Bo98 commented Oct 12, 2021

Uh oh!

Uh oh!

MikeMcQuaid commented Oct 12, 2021

Uh oh!

FnControlOption commented Oct 12, 2021

Uh oh!

BrewTestBot commented Oct 13, 2021

Uh oh!

Uh oh!

carlocab commented Oct 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carlocab commented Oct 12, 2021 •

edited

Loading

carlocab commented Oct 12, 2021 •

edited

Loading

carlocab commented Oct 12, 2021 •

edited

Loading