1892374 - `generateVMWrapper` doesn't ensure alignment of Value allocated on stack for the out parameter.

Status: UNCONFIRMED

(Core :: JavaScript Engine: JIT, defect, P2)

Description

[Mukilan Thiyagarajan]

This issue was uncovered when working on Servo's 32bit Android port. More details of that issue an be found here. Servo uses a vendored copy of SpiderMonkey (SM 115.9 as of this bug report) as its JS engine and requires the use of bindings in Rust that mirror the C++ SM smart pointers such has MutableHandle<T>. When debug_assertions are enabled in the build, the rust compiler automatically inserts assertions before every read and write operation from a raw pointer (*const T or *mut T) to ensure that the pointer address has the correct alignment for the type T.

In Servo's 32bit bit Android builds (both x86 and armv7a), we face a crash due to the pointer alignment assertion failing when constructing a MutableHandle<Value>.

01-19 18:22:28.275  3931  3961 D simpleservo: thread 'Script(1,1)' panicked at /home/mukilan/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7184f65/mozjs/src/gc/root.rs:150:28:
01-19 18:22:28.275  3931  3961 D simpleservo: misaligned pointer dereference: address must be a multiple of 0x8 but is 0xa85f2d8c

Based on the callstack, this misaligned read from MutableHandle<Value> happens when loading servo.org because in the JIT code emitted by SM's CacheIRCompiler to invoke the VM function ProxyGetPropertyByValue, the address held by the MutableHandle only has 4-byte alignment, instead of 8 byte alignment (since Value is 8 bytes even on 32bit architectures).

By debugging servo and inspecting the generated JIT code that invokes the ProxyGetPropertyByValue VM function, the root cause seems to be that generateVMWrapper for x86 and armv7 doesn't emit code to align the stack to 8-bytes when allocating storage for the out paramater (Value). Here is the gdb session confirming the same on 32bit linux:

(gdb) b /home/mukilan/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/b695c63/mozjs-sys/mozjs/js/src/jit/CacheIRCompiler.cpp:9433 if id==175
Breakpoint 4 at 0x4a36dd5: file /home/mukilan/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/b695c63/mozjs-sys/mozjs/js/src/jit/CacheIRCompiler.cpp, line 9433.
(gdb) run test.html
Thread 55 "Script(1,1)" hit Breakpoint 4, js::jit::CacheIRCompiler::callVMInternal (this=0x976c4758, masm=..., id=js::jit::VMFunctionId::ProxyGetPropertyByValue)
    at /home/mukilan/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/b695c63/mozjs-sys/mozjs/js/src/jit/CacheIRCompiler.cpp:9433
9433	  TrampolinePtr code = cx_->runtime()->jitRuntime()->getVMWrapper(id);
(gdb) step
JSContext::runtime (this=0xa7a08340) at /home/mukilan/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/b695c63/mozjs-sys/mozjs/js/src/vm/JSContext.h:376
376	  JSRuntime* runtime() { return runtime_; }
(gdb) next
js::jit::CacheIRCompiler::callVMInternal (this=0x976c4758, masm=..., id=js::jit::VMFunctionId::ProxyGetPropertyByValue)
    at /home/mukilan/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/b695c63/mozjs-sys/mozjs/js/src/jit/CacheIRCompiler.cpp:9434
9434	  MOZ_ASSERT(GetVMFunction(id).expectTailCall == NonTailCall);
(gdb) p code
$1 = {value = 0x486a58b0 "U\213\354\270@\203\240\247\213\210\254"}
(gdb) disassemble  0x486a58b0, 0x486a58cf
Dump of assembler code from 0x486a58b0 to 0x486a58cf:
   0x486a58b0:	push   %ebp
   0x486a58b1:	mov    %esp,%ebp
   0x486a58b3:	mov    $0xa7a08340,%eax
   0x486a58b8:	mov    0xac(%eax),%ecx
   0x486a58be:	mov    %esp,0x3c(%ecx)
   0x486a58c1:	        push   $0xdf91114
   0x486a58c6:	lea    0x10(%esp),%ecx
   0x486a58ca:	push   $0xffffff83
   0x486a58cc:	push   $0x0
   0x486a58ce:	mov    %esp,%edx
End of assembler dump.
(gdb) break * 0x486a58b0
Breakpoint 5 at 0x486a58b0
(gdb) c
Continuing.

Thread 55 "Script(1,1)" hit Breakpoint 5, 0x486a58b0 in ?? ()
(gdb) p $esp
$2 = (void *) 0x976c563c
(gdb) p $pc
$3 = (void (*)(void)) 0x486a58b0
(gdb) break *0x486a58ce
Breakpoint 6 at 0x486a58ce
(gdb) c
Continuing.

Thread 55 "Script(1,1)" hit Breakpoint 6, 0x486a58ce in ?? ()
(gdb) p $esp
$4 = (void *) 0x976c562c

Here 0x486a58b0 is the start of the jit code emitted by generateVMWrapper for ProxyGetByValueResult op (which calls the VM method ProxyGetPropertyByValue). As we can see above, the value of SP when we enter the function is 0x976c563c and we then push 4 32bit values onto the stack, where the last two pushes are for the Undefined Value allocated on stack for the out parameter. As expected the SP becomes 0x976c562c ( 0x976c563c - 16 bytes) which is then used as the address of the Value to be passed via the last MutableHandle out parameter of the VM function (at PC 0x486a58ce).

Comment 1

[Nicolas B. Pierron [:nbp]]

Jan, could this be caused by one of the recent patches?
I was thinking about Bug 1860541, but it is newer than Fx 115, otherwise this could be related to Bug 1774390 or Bug 1774546.

Sounds like something that might be assertable in debug builds.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #1)

Jan, could this be caused by one of the recent patches?
I was thinking about Bug 1860541, but it is newer than Fx 115, otherwise this could be related to Bug 1774390 or Bug 1774546.

I'm not sure.. this might go back further. I'll take a look soon.