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

Crash in mrb_hash_keys after hash keys are modified #3565

Closed
clayton-shopify opened this Issue Mar 29, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@clayton-shopify
Contributor

clayton-shopify commented Mar 29, 2017

The following input demonstrates a crash:

h = {}
a = []
200.times do |n|
  a[0] = n
  h[a] = 0
  puts h
end

This issue was reported by https://hackerone.com/ssarong.

The crash occurs after kh_resize increases the size of the hash table. It considers all the keys to be equal and so after the resize kh_size(h) is 1. But the one remaining mrb_hash_value still uses the old numbering, and so a crash occurs when mrb_hash_keys tries to write beyond the end of its array:

p[hv.n] = kv;

@clayton-shopify

This comment has been minimized.

Show comment
Hide comment
@clayton-shopify

clayton-shopify Mar 29, 2017

Contributor

It's also apparent from the output that insertions into the hash don't always work:

{[0]=>0}
{[1]=>0, [1]=>0}
{[2]=>0, [2]=>0, [2]=>0}
{[3]=>0, [3]=>0, [3]=>0, [3]=>0}
{[4]=>0, [4]=>0, [4]=>0, [4]=>0, [4]=>0}
{[5]=>0, [5]=>0, [5]=>0, [5]=>0, [5]=>0, [5]=>0}
{[6]=>0, [6]=>0, [6]=>0, [6]=>0, [6]=>0, [6]=>0, [6]=>0}
{[7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0}
{[8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0}         <--- incorrect
{[9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0}
...
Contributor

clayton-shopify commented Mar 29, 2017

It's also apparent from the output that insertions into the hash don't always work:

{[0]=>0}
{[1]=>0, [1]=>0}
{[2]=>0, [2]=>0, [2]=>0}
{[3]=>0, [3]=>0, [3]=>0, [3]=>0}
{[4]=>0, [4]=>0, [4]=>0, [4]=>0, [4]=>0}
{[5]=>0, [5]=>0, [5]=>0, [5]=>0, [5]=>0, [5]=>0}
{[6]=>0, [6]=>0, [6]=>0, [6]=>0, [6]=>0, [6]=>0, [6]=>0}
{[7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0, [7]=>0}
{[8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0, [8]=>0}         <--- incorrect
{[9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0, [9]=>0}
...
@ksss

This comment has been minimized.

Show comment
Hide comment
@ksss

ksss Mar 30, 2017

Contributor

I have heard from @matz.
The behavior is undefined if key objects are rewritten later (in CRuby).

Contributor

ksss commented Mar 30, 2017

I have heard from @matz.
The behavior is undefined if key objects are rewritten later (in CRuby).

@clayton-shopify

This comment has been minimized.

Show comment
Hide comment
@clayton-shopify

clayton-shopify Mar 30, 2017

Contributor

@ksss OK, so the insertion behaviour is not incorrect then, and we have some flexibility in how to fix the crash.

Contributor

clayton-shopify commented Mar 30, 2017

@ksss OK, so the insertion behaviour is not incorrect then, and we have some flexibility in how to fix the crash.

@matz matz closed this in 39ca4ef Mar 31, 2017

matz added a commit that referenced this issue Apr 1, 2017

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