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

rb_str_resize may be called on a non-String type #342

Closed
nirvdrum opened this issue Oct 28, 2017 · 2 comments
Closed

rb_str_resize may be called on a non-String type #342

nirvdrum opened this issue Oct 28, 2017 · 2 comments

Comments

@nirvdrum
Copy link

nirvdrum commented Oct 28, 2017

Revision 797ce53 introduced a change to resize strings while parsing in order to improve performance in some important cases. Unfortunately, it looks like it also attempts to resize types that aren't strings.

For example, the json_string_matching_test class that ships in the repo demonstrates a situation where internally, *result would be a Time object, not a String. I think through a remarkable set of coincidences, RSTRING_LEN called on most non-String objects happens to return 0. And rb_str_resize on a non-String object sees that object as having a length 0. Attempting to resize a 0-length string to 0 happens to head down a path that doesn't cause the process to segfault (any value > 0 would).

Assuming my analysis is correct, I think the stated performance objective can be maintained with much more safety by doing a type check:

if (RB_TYPE_P(*result, T_STRING)) {
  rb_str_resize(*result, RSTRING_LEN(*result));
}

If there's buy-in for that, I'll pull together a pull request.

I just want to state that what's in the extension does happen to work in MRI, but I think it's exploiting a set of internals (e.g., object layout) that are subject to change and could cause the whole thing to crash. If this is an acceptable risk for some additional performance, please feel free to close the issue out.

matzbot pushed a commit to ruby/ruby that referenced this issue Aug 3, 2018
* See ruby/json#342

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64177 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
@eregon
Copy link
Member

eregon commented Aug 3, 2018

I can reproduce and applied this fix in the Ruby repo: ruby/ruby@e7da0fc

hsbt pushed a commit that referenced this issue Oct 25, 2018
* See #342

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64177 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
@hsbt
Copy link
Member

hsbt commented Jun 25, 2020

Fixed at 033dd10

@hsbt hsbt closed this as completed Jun 25, 2020
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

No branches or pull requests

3 participants