Skip to content

Fix UID race condition in parallel downloads#21838

Queued
carlocab wants to merge 2 commits intomainfrom
no-drop_euid
Queued

Fix UID race condition in parallel downloads#21838
carlocab wants to merge 2 commits intomainfrom
no-drop_euid

Conversation

@carlocab
Copy link
Member

@carlocab carlocab commented Mar 26, 2026

  • 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 (excluding integration tests) for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR. Please specify below how you used AI to help you, and what steps you have taken to manually verify the changes.

On systems where EUID != UID (e.g. setuid wrappers), Utils::UID.drop_euid
calls Process::Sys.seteuid which is process-wide on macOS, causing a race
condition when parallel download threads write to directories owned by the
effective user while another thread has temporarily dropped the EUID.

Add a run_as_real_uid: option to SystemCommand that, in the forked child
only, calls Process::UID.change_privilege(Process.uid) to run as the real
UID. Use this in GitHub::API.github_cli_token and keychain_username_password
instead of wrapping the system_command call in Utils::UID.drop_euid.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com


As seen above, I used Claude to generate the changes here and the commit. I reviewed the changes carefully. Previously, brew install gcc would reliably produce errors due to the aforementioned race condition. After this change, it no longer does.

More generally, Utils::UID.drop_euid seems like something that is not safe to do with parallelism, so I intend to fix its remaining caller and remove this method.

Note: this is best reviewed while ignoring whitespace changes.

On systems where EUID != UID (e.g. setuid wrappers), `Utils::UID.drop_euid`
calls `Process::Sys.seteuid` which is process-wide on macOS, causing a race
condition when parallel download threads write to directories owned by the
effective user while another thread has temporarily dropped the EUID.

Add a `run_as_real_uid:` option to `SystemCommand` that, in the forked child
only, calls `Process::UID.change_privilege(Process.uid)` to run as the real
UID. Use this in `GitHub::API.github_cli_token` and `keychain_username_password`
instead of wrapping the `system_command` call in `Utils::UID.drop_euid`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 26, 2026 05:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a safer mechanism to run selected subprocesses under the real UID (in the forked child only) to avoid process-wide EUID changes that can race with parallel operations (e.g., parallel downloads) on macOS.

Changes:

  • Add run_as_real_uid: keyword option plumbed through system_command/SystemCommand.run APIs.
  • In SystemCommand’s forked child, optionally call Process::UID.change_privilege(Process.uid) when euid != uid.
  • Switch GitHub credential retrieval helpers to use run_as_real_uid: instead of Utils::UID.drop_euid.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
Library/Homebrew/utils/github/api.rb Updates credential/token subprocess invocations to run as real UID without process-wide EUID toggling.
Library/Homebrew/system_command.rb Introduces and implements the run_as_real_uid: option in the subprocess execution path.
Comments suppressed due to low confidence (1)

Library/Homebrew/system_command.rb:199

  • run_as_real_uid and reset_uid are both accepted and precedence is currently implicit (run_as_real_uid wins). To avoid surprising behavior for future callers, consider raising an ArgumentError when both options are set (or documenting/encoding the intended precedence explicitly).
  def initialize(executable, args: [], sudo: false, sudo_as_root: false, env: {}, input: [], must_succeed: false,
                 print_stdout: false, print_stderr: true, debug: nil, verbose: nil, secrets: [], chdir: nil,
                 reset_uid: false, run_as_real_uid: false, timeout: nil)
    require "extend/ENV"
    @executable = executable
    @args = args

    raise ArgumentError, "`sudo_as_root` cannot be set if sudo is false" if !sudo && sudo_as_root

    if print_stdout.is_a?(Symbol) && print_stdout != :debug

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@carlocab
Copy link
Member Author

More generally, Utils::UID.drop_euid seems like something that is not safe to do with parallelism, so I intend to fix its remaining caller and remove this method.

#21839

These two cannot be used at the same time. When `:run_as_real_uid` is
set, `#exec3` just ignores `:reset_uid`, so we should prevent callers
from trying to use them simultaneously.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, thanks!

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Mar 26, 2026
Any commits made after this event will not be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants