Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

utils/fork: handle termsig in safe_fork #10686

Merged
merged 1 commit into from Feb 26, 2021
Merged

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Feb 23, 2021

  • 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?

When safe_fork fails due to application termination rather than exist, exitstatus will be nil. Instead we should pass more of the object, as it has other methods like termsig for those cases. When you pass ErrorDuringExecution a Process::Status object, it will handle that and provide proper information on the failure, rather than choking because the passed exit status is nil.

It is unfortunately impossible to create a Process::Status object without using Marshal.load. There is no security risk though as this isn't user input.

Fixes #10661.

@BrewTestBot
Copy link
Member

Review period will end on 2021-02-24 at 21:42:58 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 23, 2021
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.

I'd rather we didn't use Marshall here, sorry 😭

@@ -51,7 +53,7 @@ def self.safe_fork
# to rescue them further down.
if e.is_a?(ErrorDuringExecution)
error_hash["cmd"] = e.cmd
error_hash["status"] = e.status.exitstatus
error_hash["status"] = Base64.encode64(Marshal.dump(e.status))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error_hash["status"] = Base64.encode64(Marshal.dump(e.status))
error_hash["status"] = e.status&.exitstatus

and instead never assume that this is non-nil.

Copy link
Member Author

@Bo98 Bo98 Feb 24, 2021

Choose a reason for hiding this comment

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

Fair enough on Marshal, but the problem isn't status being nil. It's exitstatus being nil. If exitstatus is nil, then we need to use termsig (see ErrorDuringExecution and its handling of a Process::Status object). So will have to change around the API a bit (both exitstatus and termsig are integers and we need to differentiate between the two if we pass it here).

Copy link
Member

Choose a reason for hiding this comment

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

Changing API around seems fine to me. As long as we assume non-zero status code I don't think preserving this is super critical (but I'll happily see a more comprehensive fix)

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 25, 2021
@BrewTestBot
Copy link
Member

Review period ended.

@Bo98 Bo98 changed the title utils/fork: marshal Process::Status object utils/fork: handle termsig in safe_fork Feb 25, 2021
@Bo98
Copy link
Member Author

Bo98 commented Feb 25, 2021

This way should be the cleanest/least disruptive.

@Bo98 Bo98 force-pushed the safe_fork-status branch 2 times, most recently from 7b6c212 to db8f9ce Compare February 25, 2021 03:02
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.

Looks good so far, nice work on this cleanup!

Library/Homebrew/utils/fork.rb Outdated Show resolved Hide resolved
Library/Homebrew/exceptions.rb Show resolved Hide resolved
Library/Homebrew/exceptions.rb Outdated Show resolved Hide resolved
Library/Homebrew/exceptions.rb Show resolved Hide resolved
Library/Homebrew/exceptions.rb Outdated Show resolved Hide resolved
@Bo98 Bo98 merged commit 06381de into Homebrew:master Feb 26, 2021
@Bo98 Bo98 deleted the safe_fork-status branch February 26, 2021 04:25
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Mar 29, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Status neither has exitstatus nor termsig.
3 participants