Repository navigation
fix(webgpu): reject non-Uint8Array byte arguments - #1832
Conversation
Signed-off-by: colinaumaty <colinaumaty@outlook.com>
ivokub
left a comment
There was a problem hiding this comment.
Thanks for the PR. Indeed, your point is valid.
Though I don't think we need so many tests to cover this fix. Could you remove the changes from .github/workflows/webgpu.yml and remove backend/accelerated/webgpu/internal/wasmruntime/runtime_test.go alltogether? Even when the tests are correct, then it just adds bloat to source code and CI work.
Signed-off-by: colinaumaty <colinaumaty@outlook.com>
Thanks for the feedback. I removed the WebGPU workflow changes and the runtime test file. The PR now only contains the byte argument validation fix. @ivokub Please review it again. |
ivokub
left a comment
There was a problem hiding this comment.
Thanks for the follow-up! Looks good!
Description
Validate JavaScript byte arguments as
Uint8Arrayvalues before accessing theirbyteLengthproperty or passing them tojs.CopyBytesToGo.Previously, primitive values could panic in
js.Value.Get, while arbitrary objects with a numericbyteLengthcould reachjs.CopyBytesToGoand panic there. These panics could terminate the Go WASM runtime instead of rejecting the promise with the intendedexpected Uint8Arrayerror.Fixes #1831
Type of change
How has this been tested?
Test A
Test B
GOOS=js GOARCH=wasm go test -exec="$(go env GOROOT)/lib/wasm/go_js_wasm_exec" ./backend/accelerated/webgpu/...GOOS=js GOARCH=wasm go vet ./backend/accelerated/webgpu/internal/wasmruntimeGOOS=js GOARCH=wasm golangci-lint run --timeout=5m ./backend/accelerated/webgpu/internal/wasmruntimeA local
go test -short ./...run was also attempted. Several unrelated cryptographic test packages reached their existing 10-minute per-package timeout; no failure involved the WebGPU/WASM packages changed here.How has this been benchmarked?
Not benchmarked. The change only adds boundary validation and regression tests.
Checklist:
golangci-lintdoes not output errors locally