Add out of bound error for wasm array access in interpreter #1823

meg-gupta · 2016-10-25T15:46:47Z

meg-gupta · 2016-10-27T00:46:30Z

@Cellule Can you please take a look ?

Cellule · 2016-10-27T08:50:09Z

test/wasm/array.js

+const moduleBytesView = new Uint8Array(blob);
+var a = Wasm.instantiateModule(moduleBytesView, {}).exports;
+print(a["goodload"](0));
+//print(a["badload"](0));


You should do a try/catch here and print failed if we don't trap

Cellule · 2016-10-27T08:50:27Z

test/wasm/array.wast

@@ -0,0 +1,7 @@
+(module


Missing copyright header, check other .wast files for example

Cellule · 2016-10-27T08:50:40Z

test/wasm/array.js

@@ -0,0 +1,6 @@
+const blob = WScript.LoadBinaryFile('array.wasm');


Missing copyright header

You should add this test to rlexe.xml you will need to add a baseline file for it too.

Cellule · 2016-10-27T08:53:52Z

lib/Runtime/Language/InterpreterStackFrame.cpp

@@ -8208,6 +8208,12 @@ const byte * InterpreterStackFrame::OP_ProfiledLoopBodyStart(const byte * ip)
        {
            BYTE* buffer = arr->GetBuffer();
            *(T2*)(buffer + index) = (T2)GetRegRaw<T2>(value);
+            return;
+        }
+        if (this->m_functionBody->IsWasmFunction())


You should do the check and trapping in OP_StArrWasm and OP_LdArrWasm, we don't want to add checks for normal asm.js

Cellule · 2016-10-27T08:54:40Z

lib/Runtime/Language/InterpreterStackFrame.cpp

+        if (this->m_functionBody->IsWasmFunction())
+        {
+            //MGTODO : Change this to throw WebAssembly.RuntimeError once implemented
+            Js::Throw::FatalInternalError();


You should throw a normal javascript runtime error for the time being so we can try/catch it in tests

Cellule · 2016-10-27T20:53:29Z

lib/Backend/amd64/LowererMDArch.cpp

@@ -1177,6 +1177,28 @@ LowererMDArch::LowerAsmJsLdElemHelper(IR::Instr * instr, bool isSimdLoad /*= fal
        Lowerer::InsertBranch(Js::OpCode::Br, doneLabel, loadLabel);
        done = doneLabel;
    }
+    else if (instr->m_func->GetJITFunctionBody()->IsWasmFunction())


I haven't looked yet at this file, but since this Pull Request is for Interpreter support, I would do this part later, also because I would prefer having x64 and x86 support at the same time

@Cellule I updated the PR with a new commit to handle x86 as well.

Cellule · 2016-10-28T00:05:54Z

Can you rebase this change on top of master and also run ch spec.js -args testsuite-bin\address.json -endargs -on:wasm in test\wasmspec.
You will have to edit spec.js to

         case 'assert_trap':
            assertTrap(m, command);
            break;

I tried real quick and it crashes in interpreter and doesn't throw with -maic:0 (aka -forcenative for asm.js code)

Cellule · 2016-10-28T00:59:14Z

I also found a case in WasmByteCodeGenerator.cpp where we emit a trap by default if there is no memory in the module

    // todo:: check for imported memory
    if (m_module->GetMemory() == nullptr)
    {
        // todo:: make that an out of bounds trap
        m_writer.EmptyAsm(Js::OpCodeAsmJs::Unreachable_Void);
    }

You should fix that as part of this change.

meg-gupta · 2016-10-28T06:01:29Z

Updated the PR to handle the case when index + sizeof the memop exceeds the heapsize. There are pending failures that need to be handled for the spec.

Right now JIT is deadstoring an operator like ( drop ( load *)), but the baseline expects a trap on this load.
The bytecode is generated such a way that when index + offset overflows, the load sees a wrapped around index and there is no trap

Cellule · 2016-10-28T23:32:28Z

Reviewed 3 of 9 files at r2, 4 of 6 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

lib/Backend/LowerMDShared.cpp, line 291 at r4 (raw file):

    {
        Lowerer::InsertAdd(true, IR::RegOpnd::New(TyMachReg, m_func), indexOpnd, IR::IntConstOpnd::New(addrOpnd->GetSize() - 1, TyMachReg, m_func), helperLabel);
        Lowerer::InsertBranch(Js::OpCode::JO, helperLabel, helperLabel);

On x64, since the index opnd is 32 bits and TyMachReg is 64bits is impossible to overflow this addition.
You could do the following check

        if (indexOpnd->GetSize() >= TySize[TyMachReg])
        {
            // Check if the addition overflowed
            Lowerer::InsertBranch(Js::OpCode::JO, helperLabel, helperLabel);
        }

lib/Backend/LowerMDShared.cpp, line 293 at r4 (raw file):

        Lowerer::InsertBranch(Js::OpCode::JO, helperLabel, helperLabel);
    }
    m_lowerer->InsertCompareBranch(indexOpnd, arrayLenOpnd, Js::OpCode::BrGe_A, true, helperLabel, helperLabel);

You have to use the result of the addition here

lib/Runtime/Language/InterpreterStackFrame.cpp, line 8291 at r4 (raw file):

            BYTE* buffer = arr->GetBuffer();
            *(ArrayType*)(buffer + index) = (ArrayType)GetRegRaw<RegType>(regSlot);
            return;

remove this return

lib/Runtime/Language/InterpreterStackFrame.cpp, line 8365 at r4 (raw file):

        const uint32 index = (uint32)GetRegRawInt(playout->SlotIndex);
        JavascriptArrayBuffer* arr = *(JavascriptArrayBuffer**)GetNonVarReg(AsmJsFunctionMemory::ArrayBufferRegister);
        if (index >= arr->GetByteLength() || index + TypeToSizeMap[playout->ViewType] > arr->GetByteLength())

@MikeHolman Do you think there is a better way to check this case.
We need to trap if you are trying to access mem[65533] for a int32 with the max length being 65536.
However, I would really to find a way to avoid 2 checks.

The only thing I can think of is
if (index > arr->GetByteLength() - TypeToSizeMap[playout->ViewType])
Since we should trap on every memory access if there is no memory, then arr->GetByteLength() > 0 in here.

lib/Runtime/Language/InterpreterStackFrame.cpp, line 8369 at r4 (raw file):

            JavascriptError::ThrowRangeError(scriptContext, JSERR_InvalidTypedArrayIndex);
        }
        (this->*LdArrFunc[playout->ViewType])(index, playout->Value);

The call to this function will do an additional check. If you can find a clean way to avoid the additional check it would be nice.
Otherwise we'll leave it as a todo for perf optimizations

test/wasm/array.js, line 9 at r4 (raw file):

const moduleBytesView = new Uint8Array(blob);
var a = Wasm.instantiateModule(moduleBytesView, {}).exports;
print(a["goodload"](0));

Could you please test the following cases
a.goodload(65535) == 0
a.goodload(65536) => trap
// Add gootstore method
a.goodstore(0) == 0
a.goodstore(65535) == 0
a.goodstore(65536) => trap

Could you also add a few cases with 32bits load/store

Comments from Reviewable

MikeHolman · 2016-10-28T23:35:15Z

    // any access below 0x10000 is safe

this is not true in wasm. WebAssembly memory can be any number of 64k pages (including 0). And AccessNeedsBoundCheck returns 0x1000000 in case IsHeapBufferConst() is false.

Refers to: lib/Backend/Lower.cpp:8847 in e15956a. [](commit_id = e15956a, deletion_comment = False)

MikeHolman · 2016-10-28T23:51:48Z

lib/Backend/LowerMDShared.cpp

+    IR::Instr *defInstr = baseOpnd->m_sym->m_instrDef;
+    IR::RegOpnd *arrayBuffer = defInstr->GetSrc1()->AsIndirOpnd()->GetBaseOpnd();
+    IR::Opnd *srcOpnd = IR::IndirOpnd::New(arrayBuffer, Js::ArrayBuffer::GetByteLengthOffset(), TyMachReg, m_func);
+    IR::RegOpnd *arrayLenOpnd = IR::RegOpnd::New(TyMachReg, m_func);


buffer length is already in src2

I don't understand, src2 of which instruction ? I am reading it from the Js::ArrayBuffer::GetByteLengthOffset() right now.

src2 of instr

s38.i32 = LdArrViewElem [s3.var+s37.i32].u8 has no src2 ? And s3/s37 don't have the length of the array buffer ?

oh, looks like on x64 it isn't set (since we didn't used to need it). in irbuilderasmjs.cpp line 1546 it should be set.

Cellule · 2016-10-28T23:53:54Z

I think we should make AsmJsJITInfo::AccessNeedsBoundCheck return return true for WebAssembly so we don't fall in that case

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1129 at r5 (raw file):

    {
        // todo:: make that an out of bounds trap
        m_writer.EmptyAsm(Js::OpCodeAsmJs::Unreachable_Void);

We should add Js::OpCodeAsmJs::MemOutOfBoundsAccess and emit this instead.

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1146 at r5 (raw file):

    if (offset >= m_module->GetMemory()->maxSize)
    {
        throw WasmCompilationException(_u("Index is out of bounds"));

I don't think this is by spec. If the previous instruction have side effect we should still trap only when we reach this load/store.
Additionally, maxSize is a page count, thus if you want to add this, you need to multiply by PAGE_SIZE.
Be careful with overflow if you do multiply by page size however .

Put this check with if (m_module->GetMemory()->minSize == 0) above

Comments from Reviewable

Cellule · 2016-10-28T23:55:48Z

Reviewed 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

lib/WasmReader/WasmByteCodeGenerator.cpp, line 1128 at r6 (raw file):

    if (m_module->GetMemory()->minSize == 0)
    {
        throw WasmCompilationException(_u("Index is out of bounds"));

This is wrong, the function is not invalid, but we need to trap accordingly if this happens.
This check is an optimization so we should emit an opcode that will trap instead.

Comments from Reviewable

MikeHolman · 2016-10-28T23:57:45Z

lib/WasmReader/WasmByteCodeGenerator.cpp

+
+    if (offset >= m_module->GetMemory()->maxSize)
+    {
+        throw WasmCompilationException(_u("Index is out of bounds"));


we shouldn't add this check. it isn't a syntax error

We are generating this bytecode sequence :
017d Ld_IntConst I3 int:-1
0183 Add_Int I2 I2 I3
0187 LdArrWasm I2 = HEAP32[I2]

When index + offset overflow, by the time we execute LdArrWasm, the value would be wrapped around. And we cant trap. We might need a different sequence here.

Adding the above check allowed me to catch such cases during bytecode generation.
Should we generate a different sequence instead ?

yeah, we probably need to emit different code here. if index+offset overflow uint32, then we need to trap at runtime. this check doesn't prevent that from happening. For example, even with small offset you might have some large index that causes index+offset to overflow

In reply to: 85627647 [](ancestors = 85627647)

We should have the check but emit an opcode that traps, that way it is a runtime trap not compile error, see my previous comment about this.

No, we can't know this at compile time. Offset+index might overflow where offset alone doesn't. Besides this doesn't work for import or grow memory

MikeHolman · 2016-10-29T00:01:48Z

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

lib/Runtime/Language/InterpreterStackFrame.cpp, line 8365 at r4 (raw file):

Previously, Cellule (Michael Ferris) wrote…

@MikeHolman Do you think there is a better way to check this case.
We need to trap if you are trying to access mem[65533] for a int32 with the max length being 65536.
However, I would really to find a way to avoid 2 checks.

The only thing I can think of is
if (index > arr->GetByteLength() - TypeToSizeMap[playout->ViewType])
Since we should trap on every memory access if there is no memory, then arr->GetByteLength() > 0 in here.

Should just be able to do check `if(index + TypeToSizeMap[playout->ViewType] > arr->GetByteLength())`, right?

Comments from Reviewable

MikeHolman · 2016-10-29T00:24:50Z

lib/Runtime/Language/InterpreterStackFrame.cpp, line 8365 at r4 (raw file):

Previously, MikeHolman (Michael Holman) wrote…

Should just be able to do check if(index + TypeToSizeMap[playout->ViewType] > arr->GetByteLength()), right?

Eh, maybe I'm wrong. I guess we need to consider case where that overflows uint32. (existing code doesn't work for that either though). as for length 0, we will have to check at runtime, as you can start with no memory or memory of size 0 and using grow memory or have a memory import. therefore, this needs to be a runtime check and cannot be assumed from time of bytecode gen

Comments from Reviewable

Cellule · 2016-10-29T03:06:45Z

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

lib/Runtime/Language/InterpreterStackFrame.cpp, line 8365 at r4 (raw file):

Previously, MikeHolman (Michael Holman) wrote…

Eh, maybe I'm wrong. I guess we need to consider case where that overflows uint32. (existing code doesn't work for that either though). as for length 0, we will have to check at runtime, as you can start with no memory or memory of size 0 and using grow memory or have a memory import. therefore, this needs to be a runtime check and cannot be assumed from time of bytecode gen

yeah you're right for length 0. the current check is correct since we check first if the index >= buffer length. We know the buffer cannot reach max uint32 (we would throw out of memory way before that) so it is safe to add a small value for the second check. I was just hoping to avoid having 2 checks.

Comments from Reviewable

Cellule · 2016-11-03T07:41:26Z

lib/Backend/IRBuilderAsmJs.cpp

@@ -3108,6 +3121,14 @@ IRBuilderAsmJs::BuildLong1Int1(Js::OpCodeAsmJs newOpcode, uint32 offset, Js::Reg
        srcOpnd = BuildSrcOpnd(srcRegSlot, TyUint32);


Js::OpCodeAsmJs::Conv_ITD & Js::OpCodeAsmJs::Conv_UTD

This looks like a bad copy paste on my part, could you remove it while you're there

Cellule · 2016-11-03T07:42:02Z

lib/Backend/i386/LowererMDArch.cpp

+    lowererMD->m_lowerer->GenerateRuntimeError(loadLabel, JSERR_InvalidTypedArrayIndex, IR::HelperOp_RuntimeRangeError);
+    Lowerer::InsertBranch(Js::OpCode::Br, loadLabel, helperLabel);
+
+    addrOpnd->AsIndirOpnd()->SetIndexOpnd(indexPair.low->AsRegOpnd());


Right now it is true that indexPair.low is a RegOpnd, but once we implement const folding for int64, the index could be a constant opnd.
Since we have unit tests hitting that path, just put an assert for now and it will be easier to fix once we get there.

Cellule · 2016-11-03T07:42:38Z

LGTM after minor fixes and rebase on master

meg-gupta · 2016-11-03T17:55:08Z

Done. Thanks @Cellule and @MikeHolman.

Out of bound memory access in Wasm traps. Code to insert relevant checks and error is added in the interpreter and jit. Wasm provides an offset + index addressing mode for memory accesses. We represent this using an int64 to address problems with wrapping due to overflow. Also new opcode LdArrViewElemWasm is added specially for wasm since we cannot use LdArrViewElem of AsmJs which is side-effect free.

… in interpreter Merge pull request #1823 from meg-gupta:indexwasm Closes #1877

msftclas added the cla-not-required label Oct 25, 2016

meg-gupta added the WebAssembly label Oct 25, 2016

meg-gupta force-pushed the indexwasm branch 2 times, most recently from 6e17fe7 to 92c27df Compare October 27, 2016 00:45

Cellule suggested changes Oct 27, 2016

View reviewed changes

Cellule reviewed Oct 27, 2016

View reviewed changes

meg-gupta force-pushed the indexwasm branch 2 times, most recently from b0bb341 to f9f7943 Compare October 28, 2016 05:51

meg-gupta force-pushed the indexwasm branch 4 times, most recently from 7986b5b to e15956a Compare October 28, 2016 22:43

meg-gupta force-pushed the indexwasm branch from e15956a to 89835b0 Compare October 28, 2016 23:49

MikeHolman reviewed Oct 28, 2016

View reviewed changes

meg-gupta force-pushed the indexwasm branch 3 times, most recently from 3bfabcd to 2d2616f Compare November 2, 2016 20:47

Cellule reviewed Nov 3, 2016

View reviewed changes

Cellule approved these changes Nov 3, 2016

View reviewed changes

meg-gupta force-pushed the indexwasm branch 2 times, most recently from 1955d37 to 3458514 Compare November 3, 2016 17:53

meg-gupta force-pushed the indexwasm branch from 3458514 to aee7f39 Compare November 3, 2016 19:21

meg-gupta force-pushed the indexwasm branch from aee7f39 to 2699947 Compare November 3, 2016 20:22

chakrabot merged commit 2699947 into chakra-core:master Nov 3, 2016

chakrabot pushed a commit that referenced this pull request Nov 3, 2016

[MERGE #1823 @meg-gupta] Add out of bound error for wasm array access…

2652fd5

… in interpreter Merge pull request #1823 from meg-gupta:indexwasm Closes #1877

This was referenced Nov 8, 2016

WASM - i64.extend_s/i32 #1864

Closed

WASM - i64.extend_u/i32 #1865

Closed

		@@ -0,0 +1,6 @@
		const blob = WScript.LoadBinaryFile('array.wasm');

		@@ -3108,6 +3121,14 @@ IRBuilderAsmJs::BuildLong1Int1(Js::OpCodeAsmJs newOpcode, uint32 offset, Js::Reg
		srcOpnd = BuildSrcOpnd(srcRegSlot, TyUint32);

Add out of bound error for wasm array access in interpreter #1823

Add out of bound error for wasm array access in interpreter #1823

Uh oh!

Conversation

meg-gupta commented Oct 25, 2016 • edited by Cellule Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meg-gupta commented Oct 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cellule commented Oct 28, 2016

Uh oh!

Cellule commented Oct 28, 2016

Uh oh!

meg-gupta commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cellule commented Oct 28, 2016

Uh oh!

MikeHolman commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cellule commented Oct 28, 2016

Uh oh!

Cellule commented Oct 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeHolman commented Oct 29, 2016

Uh oh!

MikeHolman commented Oct 29, 2016

Uh oh!

Cellule commented Oct 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cellule commented Nov 3, 2016

Uh oh!

meg-gupta commented Nov 3, 2016

Uh oh!

Uh oh!

meg-gupta commented Oct 25, 2016 •

edited by Cellule

Loading

meg-gupta Oct 27, 2016 •

edited

Loading

meg-gupta commented Oct 28, 2016 •

edited

Loading

MikeHolman commented Oct 28, 2016 •

edited

Loading