Bugzilla

The problem here is that the InterpExit stub clobbers the incoming stack args and corrupts the instance pointer.  This is platform-independent, we were possibly just lucky to catch it on Win32 because the data are packed more tightly on the stack.

It works like this: We make a call from wasm, this is routed to the InterpExit stub.  This stub executes the usual prologue setting up the exitFP, then it copies the stack arguments from the incoming frame to the outgoing frame.  Stack arguments are copied using the StackCopy routine, which just does a bit-by-bit copy.  So far so good.  However, the interp stub has only set aside 8 bytes for each argument as per the Jit ABI, and StackCopy copies 16 bytes for the V128.  When the V128 is the uppermost outgoing arg in memory and exactly abuts the incoming data (return address and instance pointer) then the copy overwrites those data.

The problem here is arguably the StackCopy routine, which I believe I factored out because I saw that the same code was used several places.  When setting up arguments for the Jit ABI it must not copy v128 data.  The non-copying case (where we box as values) handles the situation correctly, just storing a double(0) instead.  After all, passing a v128 to JS is illegal and an exception will be thrown by the callImport code.

On the subject of the callImport code: callImport_v128() throws an error immediately but I think it should not do this.  callImport() itself contains the proper check and it would be better for callImport_v128() to defer to it to throw the appropriate exception.

The problem here is that the InterpExit stub clobbers the incoming stack args and corrupts the instance pointer.  This is platform-independent, we were possibly just lucky to catch it on Win32 because the data are packed more tightly on the stack.

It works like this: We make a call from wasm, this is routed to the InterpExit stub.  This stub executes the usual prologue setting up the exitFP, then it copies the stack arguments from the incoming frame to the outgoing frame.  Stack arguments are copied using the StackCopy routine, which just does a bit-by-bit copy.  So far so good.  However, the interp stub has only set aside 8 bytes for each argument as per the Jit ABI, and StackCopy copies 16 bytes for the V128.  When the V128 is the uppermost outgoing arg in memory and exactly abuts the incoming data (return address and instance pointer) then the copy overwrites those data.  Since the instance pointer is copied after the stack arguments, garbage is moved over.

The problem here is arguably the StackCopy routine, which I believe I factored out because I saw that the same code was used several places.  When setting up arguments for the Jit ABI it must not copy v128 data.  The non-copying case (where we box as values) handles the situation correctly, just storing a double(0) instead.  After all, passing a v128 to JS is illegal and an exception will be thrown by the callImport code.

On the subject of the callImport code: callImport_v128() throws an error immediately but I think it should not do this.  callImport() itself contains the proper check and it would be better for callImport_v128() to defer to it to throw the appropriate exception.

The problem here is that the InterpExit stub clobbers the incoming stack args and corrupts the instance pointer.  This is platform-independent, we were possibly just lucky to catch it on Win32 because the data are packed more tightly on the stack.

It works like this: We make a call from wasm, this is routed to the InterpExit stub.  This stub executes the usual prologue setting up the exitFP, then it copies the stack arguments from the incoming frame to the outgoing frame.  Stack arguments are copied using the StackCopy routine, which just does a bit-by-bit copy.  So far so good.  However, the interp stub has only set aside 8 bytes for each argument as per the Jit ABI, and StackCopy copies 16 bytes for the V128.  When the V128 is the uppermost outgoing arg in memory and exactly abuts the incoming data (return address and instance pointer) then the copy overwrites those data.  Since the instance pointer is copied after the stack arguments, garbage is moved over.

The problem here is arguably the StackCopy routine, which I believe I factored out because I saw that the same code was used several places.  When setting up arguments for the Jit ABI it must not copy v128 data.  The non-copying case (where we box as values) handles the situation correctly, just storing a double(0) instead.  After all, passing a v128 to JS is illegal and an exception will be thrown by the callImport code.

On the subject of the callImport code: callImport_v128() throws an error immediately but I think it should not do this.  callImport() itself contains the proper check and it would be better for callImport_v128() to defer to it to throw the appropriate exception.  I'll make that a separate non-security bug.

The problem here is that the InterpExit stub clobbers the incoming stack args and corrupts the instance pointer.  This is platform-independent, we were possibly just lucky to catch it on Win32 because the data are packed more tightly on the stack.

It works like this: We make a call from wasm, this is routed to the InterpExit stub.  This stub executes the usual prologue setting up the exitFP, then it copies the stack arguments from the incoming frame to the outgoing frame.  Stack arguments are copied using the StackCopy routine, which just does a bit-by-bit copy.  So far so good.  However, the interp stub has only set aside 8 bytes for each argument as per the Jit ABI, and StackCopy copies 16 bytes for the V128.  When the V128 is the uppermost outgoing arg in memory and exactly abuts the incoming data (return address and instance pointer) then the copy overwrites those data.  Since the instance pointer is copied after the stack arguments, garbage is moved over.

The bug came into existence when I factored the StackCopy routine without thinking about this problem.  The non-copying case (where we box as values) handles the situation correctly, just storing a double(0) instead.  After all, passing a v128 to JS is illegal and an exception will be thrown by the callImport code. 

The fix is really in FillArgumentArrayForExit.  When setting up arguments for the Jit ABI it must not copy v128 data and should not call StackCopy.
  
On the subject of the callImport code: callImport_v128() throws an error immediately but I think it should not do this.  callImport() itself contains the proper check and it would be better for callImport_v128() to defer to it to throw the appropriate exception.  I'll make that a separate non-security bug.