How it was found

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ć