Fix patchelf concurrency issues. Show subprocess output in errors by IanButterworth · Pull Request #1410 · JuliaPackaging/BinaryBuilder.jl

IanButterworth · 2025-11-28T23:29:57Z

Summary

This PR fixes two issues in the auditor:

Improved error reporting: When patchelf or other subprocess commands fail, the error now includes the subprocess output, making debugging much easier.
Fixed race condition in patchelf operations: The auditor uses nested @threads loops—iterating over files in parallel, and within each file, iterating over libraries in parallel. When multiple threads try to relink different libraries in the same binary, they call patchelf on the same file concurrently, corrupting the ELF headers and causing "Missing ELF header" errors.

Why this wasn't an issue before

In Julia ≤1.7, nested @threads loops effectively serialized because the default :static scheduling pinned iterations to threads and didn't support nesting. Starting in Julia 1.8, @threads defaults to :dynamic scheduling (JuliaLang/julia#43919, #44136), which enables true nested parallelism—exposing the latent race condition.

i.e. in 1.7 the inner @threads becomes a standard for loop.
julia +1.7 -t6

julia> Threads.@threads for i in 1:3
           @info "outer i$i - $(Threads.threadid())"
           Threads.@threads for j in 1:3
               @info "inner j$j i$i - $(Threads.threadid())"
           end
       end
[ Info: outer i1 - 1
[ Info: outer i2 - 2
[ Info: outer i3 - 3
[ Info: inner j1 i3 - 3
[ Info: inner j2 i3 - 3
[ Info: inner j3 i3 - 3
[ Info: inner j1 i2 - 2
[ Info: inner j2 i2 - 2
[ Info: inner j3 i2 - 2
[ Info: inner j1 i1 - 1
[ Info: inner j2 i1 - 1
[ Info: inner j3 i1 - 1

julia +1.12 -t6

julia> Threads.@threads for i in 1:3
           @info "outer i$i - $(Threads.threadid())"
           Threads.@threads for j in 1:3
               @info "inner j$j i$i - $(Threads.threadid())"
           end
       end
[ Info: outer i3 - 4
[ Info: outer i1 - 6
[ Info: outer i2 - 5
[ Info: inner j1 i1 - 2
[ Info: inner j3 i3 - 6
[ Info: inner j2 i1 - 5
[ Info: inner j1 i2 - 3
[ Info: inner j2 i3 - 2
[ Info: inner j1 i3 - 4
[ Info: inner j3 i1 - 4
[ Info: inner j2 i2 - 6
[ Info: inner j3 i2 - 7

Problem

The inner loop in check_dynamic_linkage runs in parallel:

Threads.@threads for libname in collect(keys(libs))

Each iteration may call patchelf (via update_linkage, relink_to_rpath, or ensure_soname) on the same binary file (path(oh)). Without synchronization, concurrent patchelf writes corrupt the file.

Solution

Add per-file locking via with_patchelf_lock(path) to serialize patchelf operations on the same binary
Capture subprocess output and include it in error messages when commands fail

@threads

Remove nested Threads.@threads over libraries since the outer loop over files already provides parallelism. Nested threading caused: 1. Race conditions with patchelf/install_name_tool on the same file 2. ConcurrencyViolationError in OutputCollectors when multiple sandbox operations ran in parallel

This reverts commit 04d4f81.

Wrap the entire read-modify-verify cycle in with_patchelf_lock() for Linux/BSD ELF operations. This prevents concurrent ObjectFile reads from failing with EOFError when another thread is running patchelf on the same file. Split update_linkage into _update_linkage_macho and _update_linkage_elf, and ensure_soname into _ensure_soname_macho and _ensure_soname_elf for cleaner platform separation.

Sys.isbsd(platform) returns true for macOS since Darwin is BSD-based, but macOS uses install_name_tool not patchelf. Check Sys.isapple first.

giordano · 2025-11-29T11:37:05Z

        return
    end

+    # macOS uses install_name_tool (check first since Sys.isbsd is true for macOS too)


I don't understand this comment

Suggested change

# macOS uses install_name_tool (check first since Sys.isbsd is true for macOS too)

# macOS uses install_name_tool

Claude initially didn't realize isbsd covered macos. This is just a correction comment that I should've dropped

giordano · 2025-11-29T12:00:12Z

+end
+
+function _ensure_soname_macho(prefix::Prefix, path::AbstractString, platform::AbstractPlatform;
+                              verbose::Bool = false, autofix::Bool = false, subdir::AbstractString="")


The fact that this function takes autofix as argument is code smell: we shouldn't call this function to start with if autofixis false.

giordano · 2025-11-29T12:05:06Z

+    if !retval
+        @lock AUDITOR_LOGGING_LOCK @warn("Unable to set SONAME on $(rel_path)")
+        return false
+    end
+
+    # Read the SONAME back in and ensure it's set properly
+    new_soname = get_soname(path)
+    if new_soname != soname
+        @lock AUDITOR_LOGGING_LOCK @warn("Set SONAME on $(rel_path) to $(soname), but read back $(string(new_soname))!")
+        return false
+    end
+
+    if verbose
+        @lock AUDITOR_LOGGING_LOCK @info("Set SONAME of $(rel_path) to \"$(soname)\"")
+    end
+
+    return true


More duplication

I'm not really sure defining the two functions buys us anything, I'm going to remove them.

giordano · 2025-11-29T12:14:46Z

        return true
    end

+    # macOS uses install_name_tool (check first since Sys.isbsd is true for macOS too)


Fun, also weird comments are duplicated

IanButterworth mentioned this pull request Nov 28, 2025

Git: New version 2.52.0 (with debugs) JuliaPackaging/Yggdrasil#12664

Closed

giordano reviewed Nov 29, 2025

View reviewed changes

Comment thread azure-pipelines.yml Outdated

IanButterworth force-pushed the ib/show_subprocess_io branch from 82e6562 to 45aa1d5 Compare November 29, 2025 04:04

IanButterworth changed the title ~~Show subprocess output in error message when run_with_io fails~~ Fix concurrency issues. Show subprocess output in errors Nov 29, 2025

IanButterworth changed the title ~~Fix concurrency issues. Show subprocess output in errors~~ Fix patchelf concurrency issues. Show subprocess output in errors Nov 29, 2025

IanButterworth added 4 commits November 29, 2025 11:29

Show subprocess output in error message when run_with_io fails

7f2553b

Revert "Fix concurrency issues in check_dynamic_linkage"

04de88c

This reverts commit 04d4f81.

rm OutputCollectors (unused)

5406de0

giordano force-pushed the ib/show_subprocess_io branch from cb6c03e to 5280b43 Compare November 29, 2025 11:30

IanButterworth added 5 commits November 29, 2025 11:31

Add Per-file locks for patchelf operations

8190762

update to 1.12

27621e6

go back to unbuffer julia-1.7

3abcaa0

Fix platform detection: check macOS before BSD

bb5f705

Sys.isbsd(platform) returns true for macOS since Darwin is BSD-based, but macOS uses install_name_tool not patchelf. Check Sys.isapple first.

giordano force-pushed the ib/show_subprocess_io branch from 5280b43 to bb5f705 Compare November 29, 2025 11:31

giordano reviewed Nov 29, 2025

View reviewed changes

giordano force-pushed the ib/show_subprocess_io branch 2 times, most recently from ac7aa14 to f939257 Compare November 29, 2025 12:16

Reduce needless code duplication

8440c20

giordano force-pushed the ib/show_subprocess_io branch from f939257 to 8440c20 Compare November 29, 2025 12:17

Reduce needless diff

7078c65

giordano merged commit ee05c44 into master Nov 29, 2025
10 checks passed

giordano deleted the ib/show_subprocess_io branch November 29, 2025 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix patchelf concurrency issues. Show subprocess output in errors#1410

Fix patchelf concurrency issues. Show subprocess output in errors#1410
giordano merged 11 commits into
masterfrom
ib/show_subprocess_io

IanButterworth commented Nov 28, 2025 •

edited

Loading

Uh oh!

Uh oh!

giordano Nov 29, 2025

Uh oh!

IanButterworth Nov 29, 2025

Uh oh!

Uh oh!

giordano Nov 29, 2025

Uh oh!

giordano Nov 29, 2025

Uh oh!

giordano Nov 29, 2025

Uh oh!

giordano Nov 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	# macOS uses install_name_tool (check first since Sys.isbsd is true for macOS too)
	# macOS uses install_name_tool

Conversation

IanButterworth commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why this wasn't an issue before

Problem

Solution

Uh oh!

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

IanButterworth Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

giordano Nov 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

IanButterworth commented Nov 28, 2025 •

edited

Loading