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 kh_resize_iv #3388

Closed
clayton-shopify opened this Issue Jan 10, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@clayton-shopify
Contributor

clayton-shopify commented Jan 10, 2017

Supplying the following input to mirb (including the blank line at the top) results in a crash:

 
b

b('','') {}

b('','')



[]*()
b

b('','')

a=Array.new

a[102]=0
b
b

a.to_s
a




a.to_s
a.c

This appears to be a null pointer dereference in kh_resize_iv.

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

Other inputs producing the same crash also been reported by https://hackerone.com/icanthack:

def shuffle(arr)for n in 0..arr.size
g=().to_f..arr.size
L&0rescue(arr.size)
arr.to_s[0]=0and(arr.size)
arr.to_s.public_methods[0]=arr.clone.to_s.singleton_class,arr.clone.to_s,p end.class
end
def pairs(a,b)shuffle(b).p b.each{shuffle(a:private_methods)}end
first=['','']
pairs(first,['',''])
0^iterator?rescue def shuffle(arr)for n in 0..arr.size
tar0=n
arr.chr[tar0.ceil].inspect.empty?end
end
def pairs(a,b)a<<''
shuffle(b).e{x{t"",""}}end
first=['']
pairs(first,%[0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000])
@alexsnaps

This comment has been minimized.

Show comment
Hide comment
@alexsnaps

alexsnaps Jan 10, 2017

From trying to make sense out of it all, it seems like it may be something that gets GC'ed so that this iv Hash isn't around anymore. These last two examples won't SIGSEGV if the GC is disabled.

From trying to make sense out of it all, it seems like it may be something that gets GC'ed so that this iv Hash isn't around anymore. These last two examples won't SIGSEGV if the GC is disabled.

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Jan 11, 2017

Member

I could not reproduce the problem. Could you show us your configuration?

Member

matz commented Jan 11, 2017

I could not reproduce the problem. Could you show us your configuration?

@clayton-shopify

This comment has been minimized.

Show comment
Hide comment
@clayton-shopify

clayton-shopify Jan 11, 2017

Contributor

I tested all of these cases with the default MRuby configuration (nothing changed in mrbconf.h). They crash on both OS X and Ubuntu 14.04.

Note that the first test case should be supplied as input to mirb, and should include the blank line at the top. And the other two test cases should be supplied to mruby.

Contributor

clayton-shopify commented Jan 11, 2017

I tested all of these cases with the default MRuby configuration (nothing changed in mrbconf.h). They crash on both OS X and Ubuntu 14.04.

Note that the first test case should be supplied as input to mirb, and should include the blank line at the top. And the other two test cases should be supplied to mruby.

@alexsnaps

This comment has been minimized.

Show comment
Hide comment
@alexsnaps

alexsnaps Jan 11, 2017

GC.enable # crashes, works fine with GC.disabled
def shuffle(arr)for n in 0..arr.size
g=().to_f..arr.size
L&0rescue(arr.size)
arr.to_s[0]=0and(arr.size)
arr.to_s.public_methods[0]=arr.clone.to_s.singleton_class,arr.clone.to_s,p end.class
end
def pairs(a,b)shuffle(b).p b.each{shuffle(a:private_methods)}end
first=['','']
pairs(first,['',''])

This exhibits the issue on OS X (Apple LLVM version 8.0.0 (clang-800.0.42.1)) using vanilla built mruby. Fails on Ubuntu (gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609) just as well.

With some quick debugging I do see a:

Putting on 0x104b2b0 {(nil)} <- that's the iv_put on *khash_t, with its h.ed_flags nil
0x104b2b0 -> (nil) 0x104b280 0x8 0x101

and indeed, it was GC'ed earlier on:

obj_free(0x104b2b0,tt=14)

Hope this helps!

GC.enable # crashes, works fine with GC.disabled
def shuffle(arr)for n in 0..arr.size
g=().to_f..arr.size
L&0rescue(arr.size)
arr.to_s[0]=0and(arr.size)
arr.to_s.public_methods[0]=arr.clone.to_s.singleton_class,arr.clone.to_s,p end.class
end
def pairs(a,b)shuffle(b).p b.each{shuffle(a:private_methods)}end
first=['','']
pairs(first,['',''])

This exhibits the issue on OS X (Apple LLVM version 8.0.0 (clang-800.0.42.1)) using vanilla built mruby. Fails on Ubuntu (gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609) just as well.

With some quick debugging I do see a:

Putting on 0x104b2b0 {(nil)} <- that's the iv_put on *khash_t, with its h.ed_flags nil
0x104b2b0 -> (nil) 0x104b280 0x8 0x101

and indeed, it was GC'ed earlier on:

obj_free(0x104b2b0,tt=14)

Hope this helps!

@clayton-shopify

This comment has been minimized.

Show comment
Hide comment
@clayton-shopify

clayton-shopify Feb 3, 2017

Contributor

I think this may be happening because an exception object created in mrb_no_method_error (

mruby/src/error.c

Lines 526 to 527 in a746ebe

exc = mrb_funcall(mrb, mrb_obj_value(E_NOMETHOD_ERROR), "new", 3,
mrb_vformat(mrb, fmt, ap), mrb_symbol_value(id), args);
) is garbage collected during the subsequent mrb_exc_raise (
mrb_exc_raise(mrb, exc);
), since it calls mrb_exc_set which calls set_backtrace which calls back into Ruby code.

This patch seems to fix the issue:

diff --git a/src/error.c b/src/error.c
index a71ee54..90b421e 100644
--- a/src/error.c
+++ b/src/error.c
@@ -526,7 +526,9 @@ mrb_no_method_error(mrb_state *mrb, mrb_sym id, mrb_value args, char const* fmt,
   exc = mrb_funcall(mrb, mrb_obj_value(E_NOMETHOD_ERROR), "new", 3,
                     mrb_vformat(mrb, fmt, ap), mrb_symbol_value(id), args);
   va_end(ap);
+  mrb_gc_register(mrb, exc);
   mrb_exc_raise(mrb, exc);
+  mrb_gc_unregister(mrb, exc);
 }

 void

@matz Is my understanding correct?

Contributor

clayton-shopify commented Feb 3, 2017

I think this may be happening because an exception object created in mrb_no_method_error (

mruby/src/error.c

Lines 526 to 527 in a746ebe

exc = mrb_funcall(mrb, mrb_obj_value(E_NOMETHOD_ERROR), "new", 3,
mrb_vformat(mrb, fmt, ap), mrb_symbol_value(id), args);
) is garbage collected during the subsequent mrb_exc_raise (
mrb_exc_raise(mrb, exc);
), since it calls mrb_exc_set which calls set_backtrace which calls back into Ruby code.

This patch seems to fix the issue:

diff --git a/src/error.c b/src/error.c
index a71ee54..90b421e 100644
--- a/src/error.c
+++ b/src/error.c
@@ -526,7 +526,9 @@ mrb_no_method_error(mrb_state *mrb, mrb_sym id, mrb_value args, char const* fmt,
   exc = mrb_funcall(mrb, mrb_obj_value(E_NOMETHOD_ERROR), "new", 3,
                     mrb_vformat(mrb, fmt, ap), mrb_symbol_value(id), args);
   va_end(ap);
+  mrb_gc_register(mrb, exc);
   mrb_exc_raise(mrb, exc);
+  mrb_gc_unregister(mrb, exc);
 }

 void

@matz Is my understanding correct?

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Feb 4, 2017

Member

@clayton-shopify mrb_funcall() should automatically register the return value, but at least your investigation gave me a great insight. Let me think.

Member

matz commented Feb 4, 2017

@clayton-shopify mrb_funcall() should automatically register the return value, but at least your investigation gave me a great insight. Let me think.

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Feb 4, 2017

Member

Things I know now:

  • I still cannot reproduce the issue.
  • mrb_gc_register() protects the object permanently, so it's not ideal.
  • mrb_gc_unregister() is not called, since its invocation is after exception raised.
  • the return value is protected after mrb_funcall(), so we shouldn't need mrb_gc_register() nor mrb_gc_protect().
Member

matz commented Feb 4, 2017

Things I know now:

  • I still cannot reproduce the issue.
  • mrb_gc_register() protects the object permanently, so it's not ideal.
  • mrb_gc_unregister() is not called, since its invocation is after exception raised.
  • the return value is protected after mrb_funcall(), so we shouldn't need mrb_gc_register() nor mrb_gc_protect().
@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Feb 4, 2017

Member

@clayton-shopify could you insert mrb_gc_protect(mrb, exc); instead of mrb_gc_register() to see if it changes the behavior?

Member

matz commented Feb 4, 2017

@clayton-shopify could you insert mrb_gc_protect(mrb, exc); instead of mrb_gc_register() to see if it changes the behavior?

@argilo

This comment has been minimized.

Show comment
Hide comment
@argilo

argilo Feb 4, 2017

I still cannot reproduce the issue (why?)

Try running bin/mirb 3388.txt with this file: 3388.txt

It crashes for me on OS X and Ubuntu 16.04, in both cases building with clang in the default configuration.

could you insert mrb_gc_protect(mrb, exc); instead of mrb_gc_register()

The input causes a crash again when I switch to mrb_gc_protect.

argilo commented Feb 4, 2017

I still cannot reproduce the issue (why?)

Try running bin/mirb 3388.txt with this file: 3388.txt

It crashes for me on OS X and Ubuntu 16.04, in both cases building with clang in the default configuration.

could you insert mrb_gc_protect(mrb, exc); instead of mrb_gc_register()

The input causes a crash again when I switch to mrb_gc_protect.

@argilo

This comment has been minimized.

Show comment
Hide comment
@argilo

argilo Feb 4, 2017

In case it helps, here's some details from lldb:

* thread #1: tid = 492, 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292, name = 'mirb', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292
   289 	#endif
   290 	
   291 	KHASH_DECLARE(iv, mrb_sym, mrb_value, TRUE)
-> 292 	KHASH_DEFINE(iv, mrb_sym, mrb_value, TRUE, kh_int_hash_func, kh_int_hash_equal)
   293 	
   294 	typedef struct iv_tbl {
   295 	  khash_t(iv) h;
(lldb) bt
* thread #1: tid = 492, 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292, name = 'mirb', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292
    frame #1: 0x0000000000436292 mirb`kh_put_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, key=108, ret=0x0000000000000000) + 90 at variable.c:292
    frame #2: 0x0000000000436669 mirb`iv_put(mrb=0x00000000006b0010, t=0x00000000007056a0, sym=108, val=mrb_value @ 0x00007fffffffb780) + 68 at variable.c:310
    frame #3: 0x0000000000436c97 mirb`mrb_obj_iv_set(mrb=0x00000000006b0010, obj=0x00000000007056d0, sym=108, v=mrb_value @ 0x00007fffffffb7d0) + 237 at variable.c:499
    frame #4: 0x0000000000436dad mirb`mrb_iv_set(mrb=0x00000000006b0010, obj=mrb_value @ 0x00007fffffffb850, sym=108, v=mrb_value @ 0x00007fffffffb840) + 93 at variable.c:521
    frame #5: 0x000000000041ae67 mirb`exc_set_backtrace(mrb=0x00000000006b0010, exc=mrb_value @ 0x00007fffffffb880) + 247 at error.c:224
    frame #6: 0x0000000000404645 mirb`mrb_funcall_with_block(mrb=0x00000000006b0010, self=mrb_value @ 0x00007fffffffb8f0, mid=109, argc=1, argv=0x00007fffffffbb20, blk=mrb_value @ 0x00007fffffffba80) + 1964 at vm.c:430
    frame #7: 0x000000000040477b mirb`mrb_funcall_argv(mrb=0x00000000006b0010, self=mrb_value @ 0x00007fffffffbaa0, mid=109, argc=1, argv=0x00007fffffffbb20) + 78 at vm.c:447
    frame #8: 0x0000000000403e80 mirb`mrb_funcall(mrb=0x00000000006b0010, self=mrb_value @ 0x00007fffffffbae0, name="set_backtrace", argc=1) + 440 at vm.c:328
    frame #9: 0x000000000041b127 mirb`set_backtrace(mrb=0x00000000006b0010, info=mrb_value @ 0x00007fffffffbd10, bt=mrb_value @ 0x00007fffffffbd00) + 88 at error.c:259
    frame #10: 0x000000000041b2d7 mirb`mrb_exc_set(mrb=0x00000000006b0010, exc=mrb_value @ 0x00007fffffffbd40) + 299 at error.c:286
    frame #11: 0x000000000041b39d mirb`mrb_exc_raise(mrb=0x00000000006b0010, exc=mrb_value @ 0x00007fffffffbda0) + 127 at error.c:306
    frame #12: 0x000000000041c25f mirb`mrb_no_method_error(mrb=0x00000000006b0010, id=673, args=mrb_value @ 0x00007fffffffbde0, fmt="undefined method '%S' for %S") + 396 at error.c:530
    frame #13: 0x00000000004284c1 mirb`mrb_method_missing(mrb=0x00000000006b0010, name=673, self=mrb_value @ 0x00007fffffffbf50, args=mrb_value @ 0x00007fffffffbf40) + 465 at class.c:1493
    frame #14: 0x000000000042854e mirb`mrb_bob_missing(mrb=0x00000000006b0010, mod=mrb_value @ 0x00007fffffffbfb0) + 141 at class.c:1538
    frame #15: 0x00000000004070d1 mirb`mrb_vm_exec(mrb=0x00000000006b0010, proc=0x0000000000739e30, pc=0x0000000000717084) + 6999 at vm.c:1204
    frame #16: 0x0000000000405578 mirb`mrb_vm_run(mrb=0x00000000006b0010, proc=0x0000000000739e30, self=mrb_value @ 0x00007fffffffc680, stack_keep=2) + 151 at vm.c:794
    frame #17: 0x0000000000402dba mirb`main(argc=2, argv=0x00007fffffffdc78) + 1774 at mirb.c:549
    frame #18: 0x00007ffff74df830 libc.so.6`__libc_start_main(main=(mirb`main at mirb.c:361), argc=2, argv=0x00007fffffffdc78, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffdc68) + 240 at libc-start.c:291
    frame #19: 0x0000000000401dd9 mirb`_start + 41

argilo commented Feb 4, 2017

In case it helps, here's some details from lldb:

* thread #1: tid = 492, 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292, name = 'mirb', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292
   289 	#endif
   290 	
   291 	KHASH_DECLARE(iv, mrb_sym, mrb_value, TRUE)
-> 292 	KHASH_DEFINE(iv, mrb_sym, mrb_value, TRUE, kh_int_hash_func, kh_int_hash_equal)
   293 	
   294 	typedef struct iv_tbl {
   295 	  khash_t(iv) h;
(lldb) bt
* thread #1: tid = 492, 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292, name = 'mirb', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x0000000000436161 mirb`kh_resize_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, new_n_buckets=1024) + 196 at variable.c:292
    frame #1: 0x0000000000436292 mirb`kh_put_iv(mrb=0x00000000006b0010, h=0x00000000007056a0, key=108, ret=0x0000000000000000) + 90 at variable.c:292
    frame #2: 0x0000000000436669 mirb`iv_put(mrb=0x00000000006b0010, t=0x00000000007056a0, sym=108, val=mrb_value @ 0x00007fffffffb780) + 68 at variable.c:310
    frame #3: 0x0000000000436c97 mirb`mrb_obj_iv_set(mrb=0x00000000006b0010, obj=0x00000000007056d0, sym=108, v=mrb_value @ 0x00007fffffffb7d0) + 237 at variable.c:499
    frame #4: 0x0000000000436dad mirb`mrb_iv_set(mrb=0x00000000006b0010, obj=mrb_value @ 0x00007fffffffb850, sym=108, v=mrb_value @ 0x00007fffffffb840) + 93 at variable.c:521
    frame #5: 0x000000000041ae67 mirb`exc_set_backtrace(mrb=0x00000000006b0010, exc=mrb_value @ 0x00007fffffffb880) + 247 at error.c:224
    frame #6: 0x0000000000404645 mirb`mrb_funcall_with_block(mrb=0x00000000006b0010, self=mrb_value @ 0x00007fffffffb8f0, mid=109, argc=1, argv=0x00007fffffffbb20, blk=mrb_value @ 0x00007fffffffba80) + 1964 at vm.c:430
    frame #7: 0x000000000040477b mirb`mrb_funcall_argv(mrb=0x00000000006b0010, self=mrb_value @ 0x00007fffffffbaa0, mid=109, argc=1, argv=0x00007fffffffbb20) + 78 at vm.c:447
    frame #8: 0x0000000000403e80 mirb`mrb_funcall(mrb=0x00000000006b0010, self=mrb_value @ 0x00007fffffffbae0, name="set_backtrace", argc=1) + 440 at vm.c:328
    frame #9: 0x000000000041b127 mirb`set_backtrace(mrb=0x00000000006b0010, info=mrb_value @ 0x00007fffffffbd10, bt=mrb_value @ 0x00007fffffffbd00) + 88 at error.c:259
    frame #10: 0x000000000041b2d7 mirb`mrb_exc_set(mrb=0x00000000006b0010, exc=mrb_value @ 0x00007fffffffbd40) + 299 at error.c:286
    frame #11: 0x000000000041b39d mirb`mrb_exc_raise(mrb=0x00000000006b0010, exc=mrb_value @ 0x00007fffffffbda0) + 127 at error.c:306
    frame #12: 0x000000000041c25f mirb`mrb_no_method_error(mrb=0x00000000006b0010, id=673, args=mrb_value @ 0x00007fffffffbde0, fmt="undefined method '%S' for %S") + 396 at error.c:530
    frame #13: 0x00000000004284c1 mirb`mrb_method_missing(mrb=0x00000000006b0010, name=673, self=mrb_value @ 0x00007fffffffbf50, args=mrb_value @ 0x00007fffffffbf40) + 465 at class.c:1493
    frame #14: 0x000000000042854e mirb`mrb_bob_missing(mrb=0x00000000006b0010, mod=mrb_value @ 0x00007fffffffbfb0) + 141 at class.c:1538
    frame #15: 0x00000000004070d1 mirb`mrb_vm_exec(mrb=0x00000000006b0010, proc=0x0000000000739e30, pc=0x0000000000717084) + 6999 at vm.c:1204
    frame #16: 0x0000000000405578 mirb`mrb_vm_run(mrb=0x00000000006b0010, proc=0x0000000000739e30, self=mrb_value @ 0x00007fffffffc680, stack_keep=2) + 151 at vm.c:794
    frame #17: 0x0000000000402dba mirb`main(argc=2, argv=0x00007fffffffdc78) + 1774 at mirb.c:549
    frame #18: 0x00007ffff74df830 libc.so.6`__libc_start_main(main=(mirb`main at mirb.c:361), argc=2, argv=0x00007fffffffdc78, init=<unavailable>, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffdc68) + 240 at libc-start.c:291
    frame #19: 0x0000000000401dd9 mirb`_start + 41
@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Feb 4, 2017

Member

@argilo Thank you! I can reproduce now. I found out where the root cause lives.

Member

matz commented Feb 4, 2017

@argilo Thank you! I can reproduce now. I found out where the root cause lives.

@matz matz closed this in 8e0f231 Feb 4, 2017

@argilo

This comment has been minimized.

Show comment
Hide comment
@argilo

argilo Feb 4, 2017

Thanks! I tested and the issue is fixed now.

argilo commented Feb 4, 2017

Thanks! I tested and the issue is fixed now.

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