Use JSON to marshal errors from children #4752

woodruffw · 2018-08-25T18:13:43Z

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 two on #4717:

This replaces our use of Marshal to send exceptions from child processes to their controlling parents with a JSON representation of the exception state. This JSON representation is then wrapped in a new exception class, ChildProcessError. For particularly interesting exceptions in child processes (namely BuildErrors and ErrorDuringExecutions), we reify the ChildProcessError back into its original exception (which keeps analytics and other parts of the codebase that use it directly happy).

Adds tests for FormulaInstaller and Utils.safe_fork.

MikeMcQuaid

brew tests is failing.

MikeMcQuaid · 2018-08-26T11:53:06Z

Library/Homebrew/build.rb

@@ -190,7 +190,20 @@ def fixopt(f)
  build   = Build.new(formula, options)
  build.install
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_hash = JSON.parse e.to_json


Why convert this to JSON and then to a hash?

So that we can handle adding extra information in the BuildError case below -- Exception doesn't have a #to_h or a #to_hash method, so I figured it was slightly cleaner to go from JSON to a hash than to duplicate write a monkeypatched method or Utils method that does the same.

Cool, works for me, just wondering 👍

MikeMcQuaid · 2018-08-26T11:54:18Z

Library/Homebrew/dev-cmd/test.rb

+        when "Test::Unit::AssertionFailedError"
+          puts e.inner["m"]
+        else
+          puts e.inner["json_class"], e.backtrace


What happens to the exception message?

e.inner["m"] contains the message, so it still gets printed in the Test::Unit:AssertionFailedError case.

What about the other case, sorry?

I duplicated the logic from below (the rescue Exception => e), since it's basically the same failure mode (non-assertion failures, it's just that we're distinguishing between non-assertion failures in the child and parent now).

I can make it print the message explicitly if you'd like, in which case I think it makes sense to update the original fallthrough too.

Like above I think it might be nice to fiddle with handling ChildProcessError elsewhere so this special handling isn't required at all here.

MikeMcQuaid · 2018-08-26T11:55:15Z

Library/Homebrew/test/support/fixtures/failball.rb

+    prefix.install "libexec"
+
+    # This should get marshalled into a BuildError.
+    system "this_command_must_never_execute" if ENV["FAILBALL_BUILD_ERROR"]


Can you explain this line a bit?

Yup!

We're calling system with a bogus command to induce a failure, which causes it to raise a BuildError in the build process, per

brew/Library/Homebrew/formula.rb

Line 1878 in 9bc17c8

raise BuildError.new(self, cmd, args, env)

That BuildError contains a bunch of state that we use later on in analytics (and to tell the user what exactly went wrong), so we make a special case when catching that exception to re-add most of that state to the JSON blob that goes over the error pipe. The parent process reads that blob, realizes that it's a special case, and reconstructs a BuildError instead of a more generic ChildProcessError.

We do all of that only when ENV["FAILBALL_BUILD_ERROR"] is set in the test harness, so that we can fall through to the more generic test case below (when something other than a command fails in the build process, and a generic ChildProcessError gets reported back).

Can this use /usr/bin/false instead?

MikeMcQuaid · 2018-08-26T11:55:48Z

Library/Homebrew/utils/fork.rb

          read.close
          Process.wait(pid) unless socket.nil?
-          raise Marshal.load(data) unless data.nil? || data.empty? # rubocop:disable Security/MarshalLoad
+
+          error_hash = JSON.parse(data) unless data.nil? || data.empty?


Can you refactor this into an if

MikeMcQuaid · 2018-08-26T11:56:24Z

Library/Homebrew/build.rb

+  # BuildErrors are specific to build processses and not other
+  # children, which is why we create the necessary state here
+  # and not in Utils.safe_fork.
+  if e.is_a?(BuildError)


Is there any way to avoid all the is_a? Maybe looking at the keys instead?

Yeah, error_hash["json_class"] == "BuildError" should do the trick. I'll change it.

MikeMcQuaid · 2018-08-26T11:56:38Z

Library/Homebrew/utils/fork.rb

+
+          if error_hash
+            e = case error_hash["json_class"]
+            when "ErrorDuringExecution"


Any way of turning this into an actual class comparison?

Probably not at this point in the program -- we're comparing with strings because we're about to (re)create the classes that are coming from the error pipe in JSON form.

woodruffw · 2018-08-26T16:07:49Z

brew tests is failing.

Yeah looks like there's something funny going on with the mocks for Process::Status:

  4) ErrorDuringExecution#to_s when only given a command and a status to_s should eq "Failure while executing; `false` exited with 1."
     Failure/Error: its(:to_s) { is_expected.to eq "Failure while executing; `false` exited with 1." }
       expected: "Failure while executing; `false` exited with 1."
            got: "Failure while executing; `false` exited with #[InstanceDouble(Process::Status) (anonymous)]."

I'll investigate.

MikeMcQuaid

More thoughts on avoiding classes being sniffed around.

MikeMcQuaid · 2018-08-26T18:15:31Z

Library/Homebrew/build.rb

@@ -190,7 +190,20 @@ def fixopt(f)
  build   = Build.new(formula, options)
  build.install
 rescue Exception => e # rubocop:disable Lint/RescueException
-  Marshal.dump(e, error_pipe)
+  error_hash = JSON.parse e.to_json


Cool, works for me, just wondering 👍

MikeMcQuaid · 2018-08-26T18:15:53Z

Library/Homebrew/dev-cmd/test.rb

+        when "Test::Unit::AssertionFailedError"
+          puts e.inner["m"]
+        else
+          puts e.inner["json_class"], e.backtrace


What about the other case, sorry?

MikeMcQuaid · 2018-08-26T18:16:17Z

Library/Homebrew/formula_installer.rb

-    e.options = display_options(formula) if e.is_a?(BuildError)
+    # Utils.safe_fork rebuilds ErrorDuringExecutions for us, but doesn't have access
+    # to the Formula object that we need to rebuild BuildErrors.
+    if e.is_a?(ChildProcessError) && e.inner["json_class"] == "BuildError"


More duck-typing might be nice here

Yeah, I could make this do a respond_to? :inner_class if we're good on the inner_class solution above.

Actually: like above I think it might be nice to fiddle with handling ChildProcessError elsewhere so this special handling isn't required at all here.

MikeMcQuaid · 2018-08-26T18:16:22Z

Library/Homebrew/utils/fork.rb

+            # Special case: We need to recreate ErrorDuringExecutions
+            # for proper error messages and because other code expects
+            # to rescue them further down.
+            if e.is_a?(ErrorDuringExecution)


More duck-typing might be nice here

I'm a little stuck on the ducktyping here -- I don't think we want to test the exception with responds_to? here, since there are probably other exceptions that have cmd/status/output attributes that we don't want to handle specially. I flattened the exception recreation below, though.

MikeMcQuaid · 2018-08-26T18:16:53Z

Library/Homebrew/build.rb

+  # BuildErrors are specific to build processses and not other
+  # children, which is why we create the necessary state here
+  # and not in Utils.safe_fork.
+  if error_hash["json_class"] == "BuildError"


Could this become a constant that's ducktyped?

Probably not here, since this is happening in the build's child process and we haven't produced the ChildProcessError yet. I've minimized the other cases, though.

MikeMcQuaid · 2018-08-26T18:17:00Z

Library/Homebrew/dev-cmd/test.rb

        ofail "#{f.full_name}: failed"
-        puts e.message
+        case e.inner["json_class"]


Could this become a constant that's ducktyped?

Or even just a constant that's is_a?d?

I could add ChildProcessError.inner_class that just calls const_get on inner["json_class"], resulting in something like this:

if e.inner_class == ::Test::Unit::AssertionFailedError puts e.inner["m"] else puts e.inner_class, e.backtrace end

How does that sound?

Yeh, I think that'd be nicer 👍. I do wonder if this couldn't be done at a higher level somewhere (Utils.safe_fork?) so that it doesn't need to be handled as JSON in so many places.

MikeMcQuaid · 2018-08-26T18:17:26Z

Library/Homebrew/utils/fork.rb

+            error_hash = JSON.parse(data) unless data.nil? || data.empty?
+
+            e = case error_hash["json_class"]
+            when "ErrorDuringExecution"


Could this become a constant that's ducktyped?

I could probably do the same trick as mentioned in #4752 (comment), although I'm not sure how to ducktype it per se.

MikeMcQuaid · 2018-08-27T07:40:34Z

Library/Homebrew/dev-cmd/test.rb

        ofail "#{f.full_name}: failed"
-        puts e.message
+        case e.inner["json_class"]


Yeh, I think that'd be nicer 👍. I do wonder if this couldn't be done at a higher level somewhere (Utils.safe_fork?) so that it doesn't need to be handled as JSON in so many places.

MikeMcQuaid · 2018-08-27T07:41:11Z

Library/Homebrew/dev-cmd/test.rb

+        when "Test::Unit::AssertionFailedError"
+          puts e.inner["m"]
+        else
+          puts e.inner["json_class"], e.backtrace


Like above I think it might be nice to fiddle with handling ChildProcessError elsewhere so this special handling isn't required at all here.

MikeMcQuaid · 2018-08-27T07:41:43Z

Library/Homebrew/formula_installer.rb

-    e.options = display_options(formula) if e.is_a?(BuildError)
+    # Utils.safe_fork rebuilds ErrorDuringExecutions for us, but doesn't have access
+    # to the Formula object that we need to rebuild BuildErrors.
+    if e.is_a?(ChildProcessError) && e.inner["json_class"] == "BuildError"


Actually: like above I think it might be nice to fiddle with handling ChildProcessError elsewhere so this special handling isn't required at all here.

MikeMcQuaid · 2018-08-27T07:42:11Z

Library/Homebrew/test/support/fixtures/failball.rb

+    prefix.install "libexec"
+
+    # This should get marshalled into a BuildError.
+    system "this_command_must_never_execute" if ENV["FAILBALL_BUILD_ERROR"]


Can this use /usr/bin/false instead?

MikeMcQuaid · 2018-08-27T07:43:35Z

Library/Homebrew/utils/fork.rb

+                                       status: error_hash["status"],
+                                       output: error_hash["output"])
+            else
+              ChildProcessError.new(error_hash)


Could this be caught and handled somewhere elsewhere so the fact it's come from a child process doesn't need to be handled in each and every case above?

Where else were you thinking? I'm not sure where else to put it, since fork.rb is where the parent and child processes converge.

Hmm, I could do something like this:

e = ChildProcessError.new(error_hash) raise e unless e.inner_class == ErrorDuringExecution # Rebuild and throw the ErrorDuringExecution here

That flattens the logic a bit and gets rid of the ugly string comparison above, but it also creates an extra ChildProcessError that we never throw.

@woodruffw Yeh, something like that would be good, I think. It'd be good to recreate the original exception and throw it as "early" as possible so that things that are "later"/higher up the call stack can avoid needing to know or care about ChildProcessError being a thing. It's possible I've misunderstood here, though, so please feel free to argue and disagree 😆

MikeMcQuaid · 2018-08-30T07:26:03Z

@woodruffw I'm doing a bad job explaining but I think what would be nice would be a function-level handling of ChildProcessError in safe_fork which converted it into a "normal" exception so none of the callers need to care/change. Is that possible or am I missing something here?

woodruffw · 2018-08-31T00:08:02Z

I'm doing a bad job explaining but I think what would be nice would be a function-level handling of ChildProcessError in safe_fork which converted it into a "normal" exception so none of the callers need to care/change. Is that possible or am I missing something here?

If you mean a single method that converts ChildProcessErrors into BuildErrors and ErrorDuringExecutions (the two special cases): yeah, it's possible. The reason I didn't do it is because I thought it muddled the distinction between the individual callers/children a bit -- only build.rb can raise BuildErrors, for example, but sharing a single exception reconstruction method between build.rb, test.rb, and postinstall.rb makes it look like all three have the same exception behavior when they don't.

There is also at least one case where we can't rebuild the exception directly inside of safe_fork: BuildErrors need to be instantiated with the Formula that failed, but that isn't in scope until we rescue back in formula_installer.rb. The Test::Unit::AssertionFailedError, since it's a third-party library's class and takes a mystery options parameter that we don't have access to: https://www.rubydoc.info/github/test-unit/test-unit/Test/Unit/AssertionFailedError#initialize-instance_method

In sum: I could probably do something close to what you want, but I don't think it'll remove many of the caller changes I've had to make: we're still going to need to recreate the BuildError outside of safe_fork, and ChildProcessErrors are going to leak somewhere eventually (since all Exceptions in the children are made into them). It's the small price we're paying for not deserializing arbitrary classes 🙂.

MikeMcQuaid · 2018-08-31T14:36:11Z

If you mean a single method that converts ChildProcessErrors into BuildErrors and ErrorDuringExecutions (the two special cases): yeah, it's possible.

Cool. I hate to be a pain but: could we see that? Feel free to make it a bit hacky because if it's awful I may decide the current way is better?

The reason I didn't do it is because I thought it muddled the distinction between the individual callers/children a bit -- only build.rb can raise BuildErrors, for example, but sharing a single exception reconstruction method between build.rb, test.rb, and postinstall.rb makes it look like all three have the same exception behavior when they don't.

My take is that the extracting of the "real" exception from ChildProcessError feels like an implementation detail that should be handled by the methods that actually create a child process; their callers shouldn't need to care about the implementation details of the methods they call.

There is also at least one case where we can't rebuild the exception directly inside of safe_fork: BuildErrors need to be instantiated with the Formula that failed, but that isn't in scope until we rescue back in formula_installer.rb.

Could the formula object that failed be passed through instead? Alternatively, could a stub value be used here that's populated later?

ChildProcessErrors are going to leak somewhere eventually (since all Exceptions in the children are made into them). It's the small price we're paying for not deserializing arbitrary classes 🙂.

Yeh, I just think they should have a layer containing them as far into the stack as possible and they don't leak beyond that point (even if that means refactoring some existing code so everywhere they could leak from goes through a single method/class).

woodruffw · 2018-09-01T02:24:49Z

Cool. I hate to be a pain but: could we see that? Feel free to make it a bit hacky because if it's awful I may decide the current way is better?

Not a pain at all! Due diligence is appreciated 😄

I'll push a changeset up for comparison sometime tomorrow, and lay out why exactly I don't think it's the best approach when I do.

woodruffw · 2018-09-02T16:59:44Z

@MikeMcQuaid cacc5a9 has a version of your idea; I'm breaking the pros and cons out below:

Pros:

Conceptually cleaner -- ChildProcessErrors no longer occur any further up the stack than Utils.safe_fork, so individual callers don't need to handle them specially.
DRY -- all exception rewriting is now done in Utils.rewrite_child_error, which will make adding new special cases (if we ever have to) into a single-file affair. Having a single method also allows us to rewrite the exception backtraces uniformly.

Cons:

We lose access to ChildProcessError#inner_class, which we used previously to handle the AssertionFailedError case in test.rb. This isn't a huge deal since the more general (RuntimeError) case will still produce an acceptable error message and backtrace, but it makes the distinction between a test case failure and a logic error in the test code itself a little less clear.
We have to add BuildError#formula=, as the Formula that caused the BuildError isn't in scope at the time of creation within rewrite_child_error. This gets handled the same as BuildError#options=, and is just a little bit more state that adds to the conceptual load of correctly creating a BuildError.
We lose any distinction between errors that happen in child processes and those that happen in the parent. This is essentially just a rollback to the way things were when this code used Marshal so it's also not a huge deal, although I think being able to distinguish between parent and child errors could aid in debugging.

Ultimately, I writing this has changed my mind: I think the pros do outweigh the cons here, especially given that my original approach of handling ChildProcessErrors manually in each parent was pretty ugly and non-DRY.

MikeMcQuaid · 2018-09-02T20:10:15Z

Library/Homebrew/utils/fork.rb

@@ -2,6 +2,26 @@
 require "socket"

 module Utils
+  def self.rewrite_child_error(child_error)
+    error = if child_error.inner_class == ErrorDuringExecution


Maybe a case here?

case uses #=== for comparison, so comparing a class directly doesn't work 😞:

case String when String then :foo else :bar end # => :bar

MikeMcQuaid · 2018-09-02T20:11:56Z

Ultimately, I writing this has changed my mind: I think the pros do outweigh the cons here, especially given that my original approach of handling ChildProcessErrors manually in each parent was pretty ugly and non-DRY.

I agree 👍

We lose access to ChildProcessError#inner_class, which we used previously to handle the AssertionFailedError case in test.rb.

Would it be possible to create the new exception object based on that inner_class? No worries if not but I think if that's done there's no doubt this is a better approach anyway.

woodruffw · 2018-09-02T20:52:32Z

Would it be possible to create the new exception object based on that inner_class? No worries if not but I think if that's done there's no doubt this is a better approach anyway.

Probably not, since that would require arbitrary exception class instantiation (and that's what we're trying to eliminate by removing Marshal). I could add AssertionFailedError as another explicit special case, but I figured that it was safer to not try instantiating a 3rd-party exception and wouldn't add much to the error reporting anyways (since we place the ChildProcessError's message inside the RuntimeError anyways).

woodruffw · 2018-09-02T20:53:30Z

CI's failing, although it looks unrelated to these changes:

  1) brew style Homebrew::check_style_json corrected offense output format
     Failure/Error: offense_string = rubocop_result.file_offenses(formula.realpath).first.to_s
     NoMethodError:
       undefined method `upcase' for nil:NilClass
     # ./style.rb:140:in `severity_code'
     # ./style.rb:156:in `to_s'
     # ./test/cmd/style_spec.rb:54:in `block (3 levels) in <top (required)>'
     # ./test/cmd/style_spec.rb:9:in `block (2 levels) in <top (required)>'
     # ./test/spec_helper.rb:121:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:123:in `block in run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `loop'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:110:in `run'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-retry-0.6.1/lib/rspec/retry.rb:37:in `block (2 levels) in setup'
     # ./vendor/bundle/ruby/2.3.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'

MikeMcQuaid · 2018-09-03T13:26:46Z

CI's failing, although it looks unrelated to these changes:

master is not failing. Try to rebase and/or rerun the build. If it still fails: it may be related to these changes 😉

woodruffw · 2018-09-03T16:08:54Z

Rebase did the trick 🎉

MikeMcQuaid · 2018-09-03T18:43:34Z

I'm happy with this now. Any other tweaks you want before merging this @woodruffw? If not: 🚢!

woodruffw · 2018-09-03T19:38:42Z

Yeah, I'm happy with it too. I'll squash down into a few manageable/feature commits and merge in a bit.

Thanks for the review!

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.

commitay · 2018-09-03T23:17:53Z

This seems to have broken analytics.

$ brew formula-analytics go
Error: badRequest: Invalid JSON payload received. Unknown name "json_class" at 'report_requests[0].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[0].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "json_class" at 'report_requests[0].sampling_level': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[0].sampling_level': Cannot find field.
Invalid JSON payload received. Unknown name "json_class" at 'report_requests[1].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[1].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "json_class" at 'report_requests[1].sampling_level': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[1].sampling_level': Cannot find field.
Invalid JSON payload received. Unknown name "json_class" at 'report_requests[2].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[2].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "json_class" at 'report_requests[2].sampling_level': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[2].sampling_level': Cannot find field.
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/http_command.rb:218:in `check_status'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/api_command.rb:116:in `check_status'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/http_command.rb:183:in `process_response'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/batch.rb:87:in `block in decode_response_body'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/batch.rb:81:in `each_index'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/batch.rb:81:in `decode_response_body'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/http_command.rb:184:in `process_response'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/http_command.rb:299:in `execute_once'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/http_command.rb:104:in `block (2 levels) in execute'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:61:in `block in retriable'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `times'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `retriable'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/http_command.rb:101:in `block in execute'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:61:in `block in retriable'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `times'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `retriable'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/http_command.rb:93:in `execute'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.6/lib/google/apis/core/base_service.rb:178:in `batch'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:212:in `block in <top (required)>'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:209:in `each'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:209:in `each_slice'
/usr/local/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:209:in `<top (required)>'
/System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
/System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
/usr/local/Homebrew/Library/Homebrew/utils.rb:19:in `require?'
/usr/local/Homebrew/Library/Homebrew/brew.rb:95:in `<main>'

woodruffw · 2018-09-04T01:16:42Z

Ugh, thought I had fixed that. I'm guessing (since I don't know much about GA or our analytics code) that the issue here is some new keys (s and json_class) getting added to the schema.

lembacon · 2018-09-04T06:18:12Z

The core's travis test is also failing due to the breakage of analytics.

https://travis-ci.org/Homebrew/homebrew-core/builds/424184064

brew formula-analytics --days-ago=30 --json --build-error > _data/analytics/build-error/30d.json
Error: badRequest: Invalid JSON payload received. Unknown name "json_class" at 'report_requests[0].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[0].dimension_filter_clauses[0].filters[0].expressions[0]': Cannot find field.
Invalid JSON payload received. Unknown name "json_class" at 'report_requests[0].sampling_level': Cannot find field.
Invalid JSON payload received. Unknown name "s" at 'report_requests[0].sampling_level': Cannot find field.
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/http_command.rb:218:in `check_status'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/api_command.rb:116:in `check_status'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/http_command.rb:183:in `process_response'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/batch.rb:87:in `block in decode_response_body'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/batch.rb:81:in `each_index'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/batch.rb:81:in `decode_response_body'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/http_command.rb:184:in `process_response'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/http_command.rb:299:in `execute_once'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/http_command.rb:104:in `block (2 levels) in execute'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:61:in `block in retriable'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `times'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `retriable'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/http_command.rb:101:in `block in execute'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:61:in `block in retriable'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `times'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/retriable-3.1.2/lib/retriable.rb:56:in `retriable'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/http_command.rb:93:in `execute'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/vendor/ruby/ruby/2.3.0/gems/google-api-client-0.23.8/lib/google/apis/core/base_service.rb:178:in `batch'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:212:in `block in <top (required)>'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:209:in `each'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:209:in `each_slice'
/home/travis/Homebrew/Library/Taps/homebrew/homebrew-formula-analytics/cmd/brew-formula-analytics.rb:209:in `<top (required)>'
/home/travis/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
/home/travis/Homebrew/Library/Homebrew/vendor/portable-ruby/2.3.7/lib/ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
/home/travis/Homebrew/Library/Homebrew/utils.rb:19:in `require?'
/home/travis/Homebrew/Library/Homebrew/brew.rb:95:in `<main>'

woodruffw requested a review from MikeMcQuaid August 25, 2018 18:13

ghost assigned woodruffw Aug 25, 2018

ghost added the in progress Maintainers are working on this label Aug 25, 2018

MikeMcQuaid requested changes Aug 26, 2018

View reviewed changes

MikeMcQuaid reviewed Aug 26, 2018

View reviewed changes

MikeMcQuaid reviewed Aug 27, 2018

View reviewed changes

woodruffw force-pushed the error-pipe branch from 98a43ef to cacc5a9 Compare September 2, 2018 17:01

MikeMcQuaid reviewed Sep 2, 2018

View reviewed changes

woodruffw force-pushed the error-pipe branch from e046b64 to 7d1733a Compare September 3, 2018 15:45

MikeMcQuaid approved these changes Sep 3, 2018

View reviewed changes

utils: Use JSON to marshal child errors

86b9647

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.

woodruffw force-pushed the error-pipe branch from 7d1733a to 86b9647 Compare September 3, 2018 22:45

woodruffw merged commit 1eec585 into Homebrew:master Sep 3, 2018

ghost removed the in progress Maintainers are working on this label Sep 3, 2018

MikeMcQuaid mentioned this pull request Sep 4, 2018

Revert "Use JSON to marshal errors from children" #4816

Merged

woodruffw mentioned this pull request Sep 4, 2018

utils: Use JSON to marshal child errors #4819

Merged

6 tasks

lock bot added the outdated PR was locked due to age label Oct 4, 2018

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

Use JSON to marshal errors from children #4752

Use JSON to marshal errors from children #4752

Conversation

woodruffw commented Aug 25, 2018 • edited Loading

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw commented Aug 26, 2018

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Aug 30, 2018

woodruffw commented Aug 31, 2018

MikeMcQuaid commented Aug 31, 2018

woodruffw commented Sep 1, 2018

woodruffw commented Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Sep 2, 2018

woodruffw commented Sep 2, 2018

woodruffw commented Sep 2, 2018

MikeMcQuaid commented Sep 3, 2018

woodruffw commented Sep 3, 2018

MikeMcQuaid commented Sep 3, 2018

woodruffw commented Sep 3, 2018

commitay commented Sep 3, 2018

woodruffw commented Sep 4, 2018

lembacon commented Sep 4, 2018

woodruffw commented Aug 25, 2018 •

edited

Loading

woodruffw commented Sep 2, 2018 •

edited

Loading