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
Fix some errors #183
Fix some errors #183
Conversation
2ef2a03
to
08d9b5f
Compare
If something besides an object or array is passed to the parser, it will raise an exception and the lexer isn't free'd. This patch moves the token checks up a little so that we can free the lexer before raising an exception. This program will leak memory without bounds: ```ruby require 'yajl' loop do begin stream = StringIO.new('foo') projector = Yajl::Projector.new(stream) projector.project({"name" => nil}) rescue Yajl::ParseError end end ```
This commit removes token type check duplication. Before this commit, all callers of `rb_yajl_projector_filter` would do the same token type checks that the body of the function would do. This commit simply pushes the type checks down to the body of the function.
The simple value de-serialization method seems to be missing some branches in its switch statement. This adds a missing branch
This program would exhibit unbounded memory growth: ```ruby loop do begin stream = StringIO.new('[,,,,,]') projector = Yajl::Projector.new(stream) projector.project({"name" => nil}) rescue Yajl::ParseError end end ``` This patch fixes it
The projection parser should raise the same exceptions that the regular JSON parser raises when parsing a bad document
08d9b5f
to
1e50529
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for picking this up and the thorough review both 🙌
I’m super into the idea of bringing the schema down into C to avoid having to allocate the Ruby strings too 💯
|
||
VALUE val = rb_yajl_projector_filter(parser, key_schema, value_event); | ||
|
||
rb_str_freeze(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
(VALUE)args, | ||
&state); | ||
} else { | ||
yajl_lex_free(parser.lexer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch 👍
This fixes some of the errors I saw in @keithduncan's branch. I included the key freeze optimization, but none of the other optimizations I mentioned.