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

Incorrect code generation with redo inside NODE_RESCUE #3422

Closed
clayton-shopify opened this Issue Feb 1, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@clayton-shopify
Contributor

clayton-shopify commented Feb 1, 2017

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


The following code causes mruby to use up all available memory:

class A redo rescue c end

Following the execution, we see the code in codegen.c jumping between
CASE(OP_ONERR) and CASE(OP_JMP). CASE(OP_ONERR) uses realloc to double
the size of mrb->c->rescue, and since it is stuck in an infinite loop
between the two instructions, it runs out of memory.

The problematic jump is visible in the bytcode (emphasis added).

00001 NODE_SCOPE:
00001 NODE_BEGIN:
00001 NODE_CLASS:
00003 :A
00003 body:
00001 NODE_RESCUE:
00003 body:
00001 NODE_BEGIN:
00001 NODE_REDO
00003 rescue:
00003 handle classes:
00002 NODE_FCALL:
00002 NODE_SELF
00002 method='c' (670)
00003 rescue body:
00003 NODE_BEGIN:

irep 0x715200 nregs=3 nlocals=1 pools=0 syms=1 reps=1
1 000 OP_LOADNIL R1
1 001 OP_LOADNIL R2
1 002 OP_CLASS R1 :A
1 003 OP_EXEC R1 I(+1)
1 004 OP_STOP

irep 0x71b400 nregs=4 nlocals=1 pools=0 syms=2 reps=0
1 000 OP_ONERR 003 <------- Infinite loop
1 001 OP_JMP 000 <------- created here.
1 002 OP_JMP 013
1 003 OP_RESCUE R1
2 004 OP_LOADSELF R2
2 005 OP_SEND R2 :c 0
2 006 OP_MOVE R3 R1
2 007 OP_SEND R2 :=== 1
2 008 OP_JMPIF R2 010
2 009 OP_JMP 012
3 010 OP_LOADNIL R1
3 011 OP_JMP 014
3 012 OP_RAISE R1
3 013 OP_POPERR 1
3 014 OP_RETURN R1 return

Testing the same code with MRI Ruby shows that MRI Ruby rejects it as a syntax
error. It would seem MRI Ruby forbids the use of redo in the context of
rescue so the patch below disallows the related LOOP_* types. The test suite
runs successfully with the patch. The bug is mitigated inside the mruby-engine
sandbox because it triggers the instruction quota.

--- a/mrbgems/mruby-compiler/core/codegen.c
+++ b/mrbgems/mruby-compiler/core/codegen.c
@@ -2031,7 +2031,7 @@ codegen(codegen_scope *s, node *tree, int val)
     break;
 
   case NODE_REDO:
-    if (!s->loop) {
+    if (!s->loop || s->loop->type == LOOP_BEGIN || s->loop->type == LOOP_RESCUE) {
       raise_error(s, "unexpected redo");
     }
     else {

@matz matz closed this in ecb6ac8 Feb 13, 2017

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Feb 13, 2017

Member

Thank you!

Member

matz commented Feb 13, 2017

Thank you!

matz added a commit that referenced this issue Mar 11, 2017

Revert 642ab8e; ref #3422
Also fix #3499.  The issue was solved by #3462.

dkasak added a commit to dkasak/mruby that referenced this issue Mar 14, 2017

Revert 642ab8e; ref #3422
Also fix #3499.  The issue was solved by #3462.

dkasak added a commit to dkasak/mruby that referenced this issue Mar 14, 2017

Revert 642ab8e; ref #3422
Also fix #3499.  The issue was solved by #3462.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment