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: Use JSON to marshal child errors #4819

Merged
merged 1 commit into from Sep 5, 2018

Conversation

Projects
None yet
3 participants
@woodruffw
Copy link
Member

woodruffw commented Sep 4, 2018

  • 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 tests with your changes locally?

Take 3!

The only change between this and #4752 is the switch from json/add/core to json/add/exception, which should stop polluting classes other than Exception with additional information in their serialized forms.

I looked at adding a test to analytics_spec, but we don't expose the analytics blob anywhere before sending it. That's something I could add, but haven't done it yet since it would be 100% in the service of testing.

utils: Use JSON to marshal child errors
Replaces our serialization of child process
errors via Marshal with JSON, preventing
unintentional or malicious code execution outside
of the build sandbox.

Additionally, adds tests for the new behavior.

@wafflebot wafflebot bot added the in progress label Sep 4, 2018

@woodruffw woodruffw requested a review from MikeMcQuaid Sep 4, 2018

@woodruffw woodruffw merged commit eacdca8 into Homebrew:master Sep 5, 2018

3 checks passed

codecov/patch 87.23% of diff hit (target 71.28%)
Details
codecov/project 71.36% (+0.08%) compared to 817b72d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wafflebot wafflebot bot removed the in progress label Sep 5, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 5, 2018

brew formula-analytics looks good now, thanks 👍

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 7, 2018

Seeing this on build/test failures:

04:32:25 make: *** [libre.dylib] Error 1
04:32:25 /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:49:in `write': Broken pipe (Errno::EPIPE)
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:49:in `puts'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:49:in `rescue in block (3 levels) in safe_fork'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:31:in `block (3 levels) in safe_fork'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:30:in `fork'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:30:in `block (2 levels) in safe_fork'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:27:in `open'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:27:in `block in safe_fork'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0/tmpdir.rb:89:in `mktmpdir'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/utils/fork.rb:26:in `safe_fork'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/formula_installer.rb:719:in `build'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/formula_installer.rb:311:in `install'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/cmd/install.rb:321:in `install_formula'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/cmd/install.rb:253:in `block in install'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/cmd/install.rb:251:in `each'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/cmd/install.rb:251:in `install'
04:32:25 	from /usr/local/Homebrew/Library/Homebrew/brew.rb:89:in `<main>'
04:32:25 
@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 7, 2018

And can no longer manually debug builds locally:

^C/usr/local/Homebrew/Library/Homebrew/formula.rb:1847:in `gets'
Interrupt:
1. raise
2. backtrace
3. shell
Choose an action: Error: An exception occured within a build process:
  Interrupt:
/usr/local/Homebrew/Library/Homebrew/utils.rb:193:in `wait'
/usr/local/Homebrew/Library/Homebrew/utils.rb:193:in `_system'
/usr/local/Homebrew/Library/Homebrew/utils.rb:199:in `system'
/usr/local/Homebrew/Library/Homebrew/utils.rb:300:in `safe_system'
/usr/local/Homebrew/Library/Homebrew/sandbox.rb:96:in `exec'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:735:in `block in build'
/usr/local/Homebrew/Library/Homebrew/utils/fork.rb:36:in `block (3 levels) in safe_fork'
/usr/local/Homebrew/Library/Homebrew/utils/fork.rb:30:in `fork'
/usr/local/Homebrew/Library/Homebrew/utils/fork.rb:30:in `block (2 levels) in safe_fork'
/usr/local/Homebrew/Library/Homebrew/utils/fork.rb:27:in `open'
/usr/local/Homebrew/Library/Homebrew/utils/fork.rb:27:in `block in safe_fork'
/System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib/ruby/2.3.0/tmpdir.rb:89:in `mktmpdir'
/usr/local/Homebrew/Library/Homebrew/utils/fork.rb:26:in `safe_fork'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:719:in `build'
/usr/local/Homebrew/Library/Homebrew/formula_installer.rb:311:in `install'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:321:in `install_formula'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:253:in `block in install'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:251:in `each'
/usr/local/Homebrew/Library/Homebrew/cmd/install.rb:251:in `install'
/usr/local/Homebrew/Library/Homebrew/brew.rb:89:in `<main>'
==> Kept temporary files
Temporary files retained at /private/tmp/libre-20180907-13553-1ll3myj
/usr/local/Homebrew/Library/Homebrew/build.rb:207:in `close': Broken pipe (Errno::EPIPE)
	from /usr/local/Homebrew/Library/Homebrew/build.rb:207:in `rescue in <main>'
	from /usr/local/Homebrew/Library/Homebrew/build.rb:182:in `<main>'
@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Sep 7, 2018

The broken pipes are probably because we're closing the error pipe twice (once in build.rb/test.rb, and once again in Utils.safe_fork). Fixing that should be as simple as changing the write.close in safe_fork to write.close unless write.closed?, so I'll do that right now.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 7, 2018

Thanks @woodruffw.

@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Sep 7, 2018

Err, meant that we're writing inside safe_fork after we've already closed. Either way, still a short fix.

@woodruffw

This comment has been minimized.

Copy link
Member

woodruffw commented Sep 7, 2018

Following up with #4853.

@woodruffw woodruffw deleted the woodruffw-forks:error-pipe branch Sep 7, 2018

@woodruffw woodruffw referenced this pull request Sep 7, 2018

Merged

utils/fork: Check if error pipe is closed #4853

5 of 6 tasks complete

@lock lock bot added the outdated label Oct 7, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.