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

Ruby's JSON.generate should be used for payload serialization #886

Closed
waltjones opened this issue Jul 3, 2019 · 0 comments · Fixed by #890
Closed

Ruby's JSON.generate should be used for payload serialization #886

waltjones opened this issue Jul 3, 2019 · 0 comments · Fixed by #890

Comments

@waltjones
Copy link
Contributor

There was a time when Rollbar-gem saw a lot of “stack too deep” and other system errors related to JSON serialization, and when traversing the payload. There have been several fixes since then, including:

Handle recursion of payloads with cycles
#815

And, Serialization of ::Socket hangs with ActiveSupport 4.1.x - 5.1.x
#845

While there have been few “stack too deep” errors reported recently, one known issue has remained, and is documented in this test case:

context 'with recursing instance in payload and ActiveSupport is enabled' do
class Recurse
# ActiveSupport also hijacks #to_json, but relies on #as_json to do its real work.
# The implementation is different earlier vs later than 4.0, but both can
# be made to fail in the same way with this construct.
def as_json(*)
{ :self => self }
end
end
let(:payload) do
{
:key => {
:value => Recurse.new
}
}
end
let(:item) { Rollbar::Item.build_with(payload) }
it 'fails in ActiveSupport with stack too deep' do
begin
_json = item.dump
rescue NoMemoryError, SystemStackError, Java::JavaLang::StackOverflowError
# Item#dump fails with SystemStackError (ActiveSupport > 4.0)
# or NoMemoryError (ActiveSupport <= 4.0) which, as system exceptions
# not a StandardError, cannot be tested by `expect().to raise_error`
error = :SystemError
end
expect(error).to be_eql(:SystemError)
end
end

The ActiveSupport JSON serializer cannot handle cycles in its input data. The larger problem is that ActiveSupport, when installed (any app running any version of Rails, and any app using ActiveSupport as a gem), overrides both as_json and to_json in a wide swath of object types. (Moreso for as_json, which overrides virtually all Ruby objects.)

See here for to_json:
https://github.com/rails/rails/blob/b2eb1d1c55a59fee1e6c4cba7030d8ceb524267c/activesupport/lib/active_support/core_ext/object/json.rb#L48-L50

And here for as_json:
https://github.com/rails/rails/blob/b2eb1d1c55a59fee1e6c4cba7030d8ceb524267c/activesupport/lib/active_support/core_ext/object/json.rb#L52-L228

*I’m linking to a current version of ActiveSupport. The exact logic has changed over the years, and during specific versions some interesting workarounds were possible, but the basic behavior and intent has remained relatively stable.

This means that if you are using a custom serializer and also have ActiveSupport loaded, the ActiveSupport code is actually doing most of the serialization.

There is one backdoor that has been provided in >= 4.1 versions of ActiveSupport. If the caller calls to_json with a ::JSON:State object, ActiveSupport bypasses itself and calls super(). In other words, it calls Ruby’s JSON class.

https://github.com/rails/rails/blob/b2eb1d1c55a59fee1e6c4cba7030d8ceb524267c/activesupport/lib/active_support/core_ext/object/json.rb#L36-L39

Not surprisingly, this backdoor was put there specifically for Ruby’s JSON class. Ruby’s JSON.generate(obj) always calls to_json with a ::JSON:State object. So the Ruby folks didn’t want their to_json messed with and ActiveSupport accommodated.

Has this check for ::JSON:State always been there?

No, it was added in Rails 4.1. Prior to that, to_json was overridden less aggressively, yet it was still overridden in some cases. The problems caused by this apparently led to the ::JSON:State check, and allowed even more aggressive overriding without interfering with Ruby's JSON class.

Various versions of ActiveSupport allow setting your own encoding engine. Why not do that? Or call to_json with a ::JSON:State object?

  • The techniques to set the encoder have changed across versions of ActiveSupport. You’d have to check the version you’re on and do it differently for each.
  • Ruby, when using ::JSON:State, has actual state in there. (Which is probably one reason it handles cycles!) You’d have to reimplement the behavior of Ruby JSON.
  • All these things are workarounds to cause Ruby’s JSON encoder to get used. Why not just call JSON.generate? It’s the one encoder that works correctly.

Aren’t there other JSON gems that don’t have this issue?

Most of the available options rely on to_json or as_json (or both). If ActiveSupport is present, it will inject itself. The above linked test case uses MultiJson using Oj, which promptly calls ActiveSupport and crashes.

Is Ruby JSON less performant?

These other libraries end up calling out to ActiveSupport anyway. Ruby JSON is not less performant than ActiveSupport.

Because of MultiJson, users can currently select their JSON engine. Is removing this a breaking change?

Users cannot currently select their JSON engine. Looking at:
https://github.com/rollbar/rollbar-gem/blob/v2.20.2/lib/rollbar/json.rb#L32-L37

MultiJson uses its default adapter, unless Oj is present, in which case it uses Oj no matter what. Currently, if an app loads Oj for any other reason, it cannot stop Rollbar-gem from trying to use it. This isn’t ideal, but it’s especially problematic when taking the ActiveSupport problem into account. At least if the user could select their JSON engine, they could select Ruby’s own JSON. Using MultiJson though, even trying to select the core Ruby encoder, it goes through MultiJson’s json_common, and crashes.

To date, what JSON engine the users gets has been random:

  1. Whatever MultiJson’s current default is, unless
  2. Oj is present anywhere, unless
  3. ActiveSupport is present anywhere.

Unfortunately, for most users (3) has been the default state.

Don’t some gems replace ::JSON?

Yes, and it’s a terrible idea, similar to replacing String or Array. Rollbar-gem doesn’t do this, and if other code in an app is replacing core Ruby classes, that’s outside the scope of what this gem can fix or be responsible for.

Recommendation:

Use JSON.generate for serialization. It’s standard and stable across all versions of Ruby. It requires no external dependencies. And it works.

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 a pull request may close this issue.

1 participant