Segfault in mruby, mruby_engine and the parent MRI Ruby due to null pointer dereference

Introduction ============ Certain valid Ruby programs are able to cause a segmentation fault in mruby through a null pointer derefence, which in turn leads to a crash in mruby_engine and the parent MRI Ruby process. Proof of concept ================ crash.rb: --------- def method yield end method(&a &&= 0) 1. Save the above code as crash.rb 2. Run either: a) mruby crash.rb b) sandbox crash.rb 3. Both cause a segmentation fault. Discussion ========== Everything below assumes the latest master of the mruby repository as of Nov 12th, which is commit `88604e39ac9c25ffdad2e3f03be26516fe866038`. The null pointer dereference itself for the above POC happens in `ext/mruby_engine/mruby/src/vm.c`, line 1266: regs[0] = m->env->stack[0]; The `env` member of is a null pointer since `m` refers to a `RProc *` of a non-closure lambda. The underlying cause of the bug is an unsafe peephole optimization during code generation which isn't a valid transformation in certain contexts. Let's examine the debug information generated by running `mruby -v crash.rb`: 00001 NODE_SCOPE: 00005 local variables: 00005 a 00001 NODE_BEGIN: 00001 NODE_DEF: 00003 method 00002 NODE_BEGIN: 00002 NODE_YIELD: 00005 NODE_CALL: 00005 NODE_SELF 00005 method='method' (665) 00005 args: 00005 block: 00005 NODE_BLOCK_ARG: 00005 NODE_OP_ASGN: 00005 lhs: 00005 NODE_LVAR a 00005 op='&&' (666) 00005 NODE_INT 0 base 10 irep 0x10d5920 nregs=4 nlocals=2 pools=0 syms=1 reps=1 file: crash.rb 1 000 OP_TCLASS R2 1 001 OP_LAMBDA R3 I(+1) 1 1 002 OP_METHOD R2 :method 5 003 OP_LOADSELF R2 5 004 OP_JMPNOT R1 007 5 005 OP_LOADI R3 0 5 006 OP_MOVE R1 R3 ; R1:a 5 007 OP_SENDB R2 :method 0 5 008 OP_STOP irep 0x10db740 nregs=3 nlocals=2 pools=0 syms=1 reps=0 file: crash.rb 1 000 OP_ENTER 0:0:0:0:0:0:0 2 001 OP_BLKPUSH R2 0:0:0:0 2 002 OP_SEND R2 :call 0 2 003 OP_RETURN R2 return From the above, it can be seen that R3 is set to a lambda representing the method `method` but then never re-set again in the true branch of the `JMPNOT`. Furthermore, the condition of `JMPNOT` will always be true in this particular case since `a` was never defined so the use of the &&= assignment operator causes it to be set to `nil`. Since a method called through `SENDB A B C` expects the passed block to be located in the register A+C+1, which is R3 in the caller's context, it is obvious the method will instead receive the lambda representing `method` instead. Since lambdas are represented as `(RProc *)`, just as blocks are, this won't cause a type error. However, this particular lambda is not a closure so its `(RProc *)` doesn't contain an `env` member and it is a null pointer. Note that the problem isn't limited to methods with blocks, as can be seen from this slightly modified example which also causes a segfault: modified_crash.rb: ------------------ def method(x) x.call end method(a &&= 0) We then investigated further to check why the code generator was producing faulty bytecode, only to find that it in fact emits a correct `MOVE R3 R1` instruction immediately after the `LOADSELF`. However, since it is then followed by an appropriately "shaped" `JMPNOT`, it triggers the following peephole reduction rule which elides the `MOVE`: MOVE R3 R1 JMPNOT R3 0 -------------- JMPNOT R1 0 The rule in question is located in `ext/mruby_engine/mruby/mrbgems/mruby-compiler/core/codegen.c`, line 350. In another related example where the operation `a &&= 0` is done outside the argument list, the bytecode is almost exactly the same, just shuffled around. However, this shuffling prevents the `MOVE` and `JMPNOT` in being adjacent, which prevents the peephole rule from triggering and results in an ordinary mruby exception: non_crash.rb: ------------- def method yield end a &&= 0 method(&a) This yields the following AST and bytecode: 00001 NODE_SCOPE: 00005 local variables: 00005 a 00001 NODE_BEGIN: 00001 NODE_DEF: 00003 method 00002 NODE_BEGIN: 00002 NODE_YIELD: 00005 NODE_OP_ASGN: 00005 lhs: 00005 NODE_LVAR a 00005 op='&&' (666) 00005 NODE_INT 0 base 10 00007 NODE_CALL: 00007 NODE_SELF 00007 method='method' (665) 00007 args: 00007 block: 00007 NODE_BLOCK_ARG: 00007 NODE_LVAR a irep 0x2468920 nregs=4 nlocals=2 pools=0 syms=1 reps=1 file: a-new-kind-of-crash.7 1 000 OP_TCLASS R2 1 001 OP_LAMBDA R3 I(+1) 1 1 002 OP_METHOD R2 :method 5 003 OP_JMPNOT R1 005 5 004 OP_LOADI R1 0 ; R1:a 7 005 OP_LOADSELF R2 7 006 OP_MOVE R3 R1 ; R1:a <<<the MOVE isn't elided>>> 7 007 OP_SENDB R2 :method 0 7 008 OP_STOP irep 0x246e740 nregs=3 nlocals=2 pools=0 syms=1 reps=0 file: a-new-kind-of-crash.7 1 000 OP_ENTER 0:0:0:0:0:0:0 2 001 OP_BLKPUSH R2 0:0:0:0 2 002 OP_SEND R2 :call 0 2 003 OP_RETURN R2 return trace: [1] a-new-kind-of-crash.7:2:in Object.method [0] a-new-kind-of-crash.7:7 LocalJumpError: unexpected yield Solution ======== We investigated several possible solutions, but ultimately the peephole optimization in question seems very precarious. As best as we could tell, the triggering factor seems to be an AST with a `NODE_OP_ASGN` nested inside a `NODE_CALL` (not necessarily as an immediate child). Since most of the work is done in the function `codegen` which is called recursively, there is no simple way to detect this special case without examining the AST. Therefore, presumably a flag should be set when the code generator enters a `NODE_CALL` so the peepholer knows not to make this optimization if inside it. Since with our limited knowledge of the codebase it's not obvious that this is the right solution, we decided to simply disable the optimization for now since it is relatively low impact. We supply the following patch: diff --git a/mrbgems/mruby-compiler/core/codegen.c b/mrbgems/mruby-compiler/core/codegen.c index 9b064b8..6539ed4 100644 --- a/mrbgems/mruby-compiler/core/codegen.c +++ b/mrbgems/mruby-compiler/core/codegen.c @@ -346,13 +346,6 @@ genop_peep(codegen_scope *s, mrb_code i, int val) } } break; - case OP_JMPIF: - case OP_JMPNOT: - if (c0 == OP_MOVE && GETARG_A(i) == GETARG_A(i0)) { - s->iseq[s->pc-1] = MKOP_AsBx(c1, GETARG_B(i0), GETARG_sBx(i)); - return s->pc-1; - } - break; default: break; } With the optimization disabled, the segfault no longer happens and the code passes all tests. If the mruby developers insist that this optimization should remain, we are willing to work with them to develop a fix. -- Denis Kasak Damir Jelić