From c59a42b24e660632d2984b74f2e8a78532634886 Mon Sep 17 00:00:00 2001 From: Carlo Cabrera <30379873+carlocab@users.noreply.github.com> Date: Sat, 3 Jul 2021 19:23:19 +0100 Subject: [PATCH 1/3] formula_cellar_checks: check for `cpuid` instruction when needed This implements the second audit discussed in #11608. --- Library/Homebrew/extend/os/mac/keg.rb | 6 +++ Library/Homebrew/formula_cellar_checks.rb | 46 +++++++++++++++++++++++ Library/Homebrew/keg.rb | 4 ++ 3 files changed, 56 insertions(+) diff --git a/Library/Homebrew/extend/os/mac/keg.rb b/Library/Homebrew/extend/os/mac/keg.rb index 504b5c134f..f41cd6c9e7 100644 --- a/Library/Homebrew/extend/os/mac/keg.rb +++ b/Library/Homebrew/extend/os/mac/keg.rb @@ -19,4 +19,10 @@ class Keg GENERIC_MUST_BE_WRITABLE_DIRECTORIES + [HOMEBREW_PREFIX/"Frameworks"] ).sort.uniq.freeze + + undef binary_executable_or_library_files + + def binary_executable_or_library_files + mach_o_files + end end diff --git a/Library/Homebrew/formula_cellar_checks.rb b/Library/Homebrew/formula_cellar_checks.rb index d1601f924d..8d7a4b4c1e 100644 --- a/Library/Homebrew/formula_cellar_checks.rb +++ b/Library/Homebrew/formula_cellar_checks.rb @@ -284,6 +284,39 @@ module FormulaCellarChecks "Service command does not exist" unless File.exist?(formula.service.command.first) end + def check_cpuid_instruction(formula) + return unless formula.prefix.directory? + # TODO: add methods to `utils/ast` to allow checking for method use + return unless formula.path.read.include? "ENV.runtime_cpu_detection" + # Checking for `cpuid` only makes sense on Intel: + # https://en.wikipedia.org/wiki/CPUID + return unless Hardware::CPU.intel? + + # macOS `objdump` is a bit slow, so we prioritise llvm's `llvm-objdump` (~5.7x faster) + # or binutils' `objdump` (~1.8x faster) if they are installed. + objdump = Formula["llvm"].opt_bin/"llvm-objdump" if Formula["llvm"].any_version_installed? + objdump ||= Formula["binutils"].opt_bin/"objdump" if Formula["binutils"].any_version_installed? + objdump ||= which("objdump") + objdump ||= which("objdump", ENV["HOMEBREW_PATH"]) + objdump ||= begin + # If the system provides no `objdump`, install binutils instead of llvm since + # binutils is smaller and has fewer dependencies. + ohai "Installing `binutils` for `cpuid` instruction check..." + safe_system HOMEBREW_BREW_FILE, "install", "binutils" + Formula["binutils"].opt_bin/"objdump" + end + + keg = Keg.new(formula.prefix) + has_cpuid_instruction = false + keg.binary_executable_or_library_files.each do |file| + has_cpuid_instruction = cpuid_instruction?(file, objdump) + break if has_cpuid_instruction + end + return if has_cpuid_instruction + + "No `cpuid` instruction detected. #{formula} should not use `ENV.runtime_cpu_detection`." + end + def audit_installed @new_formula ||= false @@ -303,6 +336,7 @@ module FormulaCellarChecks problem_if_output(check_shim_references(formula.prefix)) problem_if_output(check_plist(formula.prefix, formula.plist)) problem_if_output(check_python_symlinks(formula.name, formula.keg_only?)) + problem_if_output(check_cpuid_instruction(formula)) end alias generic_audit_installed audit_installed @@ -311,6 +345,18 @@ module FormulaCellarChecks def relative_glob(dir, pattern) File.directory?(dir) ? Dir.chdir(dir) { Dir[pattern] } : [] end + + def cpuid_instruction?(file, objdump = "objdump") + has_cpuid_instruction = false + Utils.popen_read(objdump, "--disassemble", file) do |io| + until io.eof? + has_cpuid_instruction = io.readline.include? "cpuid" + break if has_cpuid_instruction + end + end + + has_cpuid_instruction + end end require "extend/os/formula_cellar_checks" diff --git a/Library/Homebrew/keg.rb b/Library/Homebrew/keg.rb index c5bad0889b..2bcf1eae69 100644 --- a/Library/Homebrew/keg.rb +++ b/Library/Homebrew/keg.rb @@ -525,6 +525,10 @@ class Keg find { |pn| FileUtils.rm_rf pn if pn.basename.to_s == "__pycache__" } end + def binary_executable_or_library_files + elf_files + end + private def resolve_any_conflicts(dst, dry_run: false, verbose: false, overwrite: false) From 63aa1920883841fb0d6eb0c5513de63406d91879 Mon Sep 17 00:00:00 2001 From: Carlo Cabrera <30379873+carlocab@users.noreply.github.com> Date: Mon, 5 Jul 2021 17:47:20 +0100 Subject: [PATCH 2/3] Incorporate suggestions from feedback 1. Never install `binutils`. Instead, report an audit failure. 2. Tighten instruction check with a stricter matching strategy. --- Library/Homebrew/formula_cellar_checks.rb | 28 ++++++++++++++--------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/Library/Homebrew/formula_cellar_checks.rb b/Library/Homebrew/formula_cellar_checks.rb index 8d7a4b4c1e..a10dfb6f60 100644 --- a/Library/Homebrew/formula_cellar_checks.rb +++ b/Library/Homebrew/formula_cellar_checks.rb @@ -298,19 +298,17 @@ module FormulaCellarChecks objdump ||= Formula["binutils"].opt_bin/"objdump" if Formula["binutils"].any_version_installed? objdump ||= which("objdump") objdump ||= which("objdump", ENV["HOMEBREW_PATH"]) - objdump ||= begin - # If the system provides no `objdump`, install binutils instead of llvm since - # binutils is smaller and has fewer dependencies. - ohai "Installing `binutils` for `cpuid` instruction check..." - safe_system HOMEBREW_BREW_FILE, "install", "binutils" - Formula["binutils"].opt_bin/"objdump" + + unless objdump + return <<~EOS + No `objdump` found, so cannot check for a `cpuid` instruction. Install `objdump` with + brew install binutils + EOS end keg = Keg.new(formula.prefix) - has_cpuid_instruction = false - keg.binary_executable_or_library_files.each do |file| - has_cpuid_instruction = cpuid_instruction?(file, objdump) - break if has_cpuid_instruction + has_cpuid_instruction = keg.binary_executable_or_library_files.any? do |file| + cpuid_instruction?(file, objdump) end return if has_cpuid_instruction @@ -347,10 +345,18 @@ module FormulaCellarChecks end def cpuid_instruction?(file, objdump = "objdump") + @instruction_column_index ||= {} + @instruction_column_index[objdump] ||= if Utils.popen_read(objdump, "--version").include? "LLVM" + 1 # `llvm-objdump` or macOS `objdump` + else + 2 # GNU binutils `objdump` + end + has_cpuid_instruction = false Utils.popen_read(objdump, "--disassemble", file) do |io| until io.eof? - has_cpuid_instruction = io.readline.include? "cpuid" + instruction = io.readline.split("\t")[@instruction_column_index[objdump]]&.chomp + has_cpuid_instruction = instruction.match?(/^cpuid(\s+|$)/) if instruction break if has_cpuid_instruction end end From 5ed4430daf8108462126cbcdf3088f2ec887a74e Mon Sep 17 00:00:00 2001 From: Carlo Cabrera <30379873+carlocab@users.noreply.github.com> Date: Mon, 5 Jul 2021 10:12:34 -0700 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Rylan Polster --- Library/Homebrew/formula_cellar_checks.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/formula_cellar_checks.rb b/Library/Homebrew/formula_cellar_checks.rb index a10dfb6f60..a317967731 100644 --- a/Library/Homebrew/formula_cellar_checks.rb +++ b/Library/Homebrew/formula_cellar_checks.rb @@ -307,10 +307,9 @@ module FormulaCellarChecks end keg = Keg.new(formula.prefix) - has_cpuid_instruction = keg.binary_executable_or_library_files.any? do |file| + return if keg.binary_executable_or_library_files.any? do |file| cpuid_instruction?(file, objdump) end - return if has_cpuid_instruction "No `cpuid` instruction detected. #{formula} should not use `ENV.runtime_cpu_detection`." end @@ -355,8 +354,8 @@ module FormulaCellarChecks has_cpuid_instruction = false Utils.popen_read(objdump, "--disassemble", file) do |io| until io.eof? - instruction = io.readline.split("\t")[@instruction_column_index[objdump]]&.chomp - has_cpuid_instruction = instruction.match?(/^cpuid(\s+|$)/) if instruction + instruction = io.readline.split("\t")[@instruction_column_index[objdump]]&.strip + has_cpuid_instruction = instruction == "cpuid" if instruction.present? break if has_cpuid_instruction end end