Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

amd64: fixes memory.fill bug #1055

Merged
merged 3 commits into from
Jan 23, 2023
Merged

amd64: fixes memory.fill bug #1055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1c02ad2
amd64: fixes memory.fill bug
mathetake Jan 23, 2023
ae94ccb
fix: ANDL 0xff value.register
evacchi Jan 23, 2023
a743eaa
guard on `!platform.CompilerSupported()`
evacchi Jan 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions internal/engine/compiler/impl_amd64.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3799,6 +3799,8 @@ func (c *amd64Compiler) compileFillLoopImpl(destinationOffset, value, fillSize *
emptyEightGroupsJump := c.assembler.CompileJump(amd64.JEQ)

if replicateByte {
// Truncate value.register to a single byte
c.assembler.CompileConstToRegister(amd64.ANDQ, 0xff, value.register)
// Replicate single byte onto full 8-byte register.
c.assembler.CompileConstToRegister(amd64.MOVQ, 0x0101010101010101, tmp)
c.assembler.CompileRegisterToRegister(amd64.IMULQ, tmp, value.register)
Expand Down
20 changes: 20 additions & 0 deletions internal/integration_test/fuzzcases/fuzzcases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"testing"

"github.com/tetratelabs/wazero"
"github.com/tetratelabs/wazero/api"
"github.com/tetratelabs/wazero/internal/platform"
"github.com/tetratelabs/wazero/internal/testing/require"
"github.com/tetratelabs/wazero/internal/wasm"
Expand Down Expand Up @@ -383,3 +384,22 @@ func Test888(t *testing.T) {
require.NoError(t, err)
})
}

func Test1054(t *testing.T) {
if !platform.CompilerSupported() {
return
}

modules := make([]api.Module, 0, 2)
run(t, func(t *testing.T, r wazero.Runtime) {
mod, err := r.InstantiateModuleFromBinary(ctx, getWasmBinary(t, 1054))
require.NoError(t, err)
modules = append(modules, mod)
})

// Checks if the memory state is the same between engines.
require.Equal(t,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this out-of-bounds is due to how run() works. it is called up to two times, depending on if platform.CompilerSupported()

so my guess is this test is only really going to do anything if platform.CompilerSupported() as that's the only time you'll have two modules. maybe guard on it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added at the top:

	if !platform.CompilerSupported() {
		return
	}

modules[0].Memory().(*wasm.MemoryInstance).Buffer,
modules[1].Memory().(*wasm.MemoryInstance).Buffer,
)
}
Binary file added internal/integration_test/fuzzcases/testdata/1054.wasm
View file
Open in desktop
Binary file not shown.
11 changes: 11 additions & 0 deletions internal/integration_test/fuzzcases/testdata/1054.wat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
(module
(type (;0;) (func))
(func (;0;) (type 0)
i32.const 1000
i32.const 1000
i32.const 1000
Comment on lines +4 to +6
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like if these values exceed one byte range (>0xff) this happens, that's all I narrowed down!

Copy link
Contributor

@evacchi evacchi Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting, the reason why the bug does not occur on aarch64 is less trivial than you may think.

Maybe the spec is a bit unclear, because memory.fill takes 3 args:

  • dest: destination address
  • value: byte value to repeat
  • width: offset from dst

if value > 0xff then things start to get funky for us. But value is the byte that should fill the memory from address dest to offset width. Let me stress here it is supposed to be a byte. If value is anything but a byte it is unclear to me whether we should bitcast or trap (and I couldn't find references).

Proof

I have dumped the contents of the memory buffer at 999:2001; on M1, for value:=255 we get:

"\x00\xff\xff\xff\xff\xff\xff\ ... \xff\xff\x00"
"\x00\xff\xff\xff\xff\xff\xff\ ... \xff\xff\x00"

we get the same on amd64.

Now, let's set value:=256 on amd64:

"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00..."
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x01\x01\x01\x01\x01\x01\x00\x01\x01\x01\x01\x01\x01\x01\x00\x01\x01\x01\x01\x01\x01\x01\x00..."

which is obviously incorrect. But what happens instead on M1?

"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00..."
"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00..."

which is equal... because:

(1) we are casting to byte in the interpreter

value := byte(ce.popValue())

(2) on aarch64 STRB takes (I think) the least significant byte from the 32-bit register

(3) on amd64, the bug is only triggered for width>15 because compileMemoryFill "uses efficient REP STOSQ instructions to copy in quadword (8 bytes) batches if the size if above 15 bytes"

Copy link
Contributor

@evacchi evacchi Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, the fix is to ensure that value is 1 byte before IMULQ value to 0x0101010101010101, which can be done by ANDQ 0xff value.register

memory.fill
)
(memory (;0;) 1 2)
(start 0)
)