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.

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

bytes: IndexByte can return -4294967295 when memory usage is above 2^31 on js/wasm #65571

Closed
Nemikolh opened this issue Feb 7, 2024 · 7 comments
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Nemikolh
Copy link

Nemikolh commented Feb 7, 2024

Go version

1.21.7

Output of go env in your module/workspace:

GOARCH=wasm
GOOS=js
GOHOSTARCH=amd64

(I'll update this with the values I used when experimenting with a fix when I can)

What did you do?

It seems that bytes.IndexByte when compiled to js/wasm can return values that are below -1. The code snippet that triggered this issue is this:

result := []byte(strconv.FormatFloat(absValue, 'g', -1, 64))
// ...
dot := bytes.IndexByte(result, '.')

(extracted from https://github.com/evanw/esbuild/blob/2af5ccf478812d2d7226ad4435d46fbbb3419a8c/internal/js_printer/js_printer.go#L3464)

Original issue: stackblitz/webcontainer-core#1310

I haven't been able to create a minimal example exhibiting the same behaviour just yet. I'm still working on that. 👈

However the bug randomly appears when memory usage gets near 2^31. I have also verified that the issue is present both in Chrome and Firefox (so this does not appear to be a WASM vm bug)

What did you see happen?

bytes.IndexByte returns -4294967295 on the following snippet:

dot := bytes.IndexByte([]byte(strconv.FormatFloat(0.5, 'g', -1, 64)), '.')

I've also seen similar values such as -4294967293 for other input.

What did you expect to see?

I expect to see the correct, positive values and no other negative values than -1.

Note that I did a simple patch modifying:

https://github.com/golang/go/blob/master/src/bytes/bytes.go#L95-L97

to be:

func IndexByte(b []byte, c byte) int {
	res := bytealg.IndexByte(b, c)
	if res < -1 {
		return res + 1 << 32
	}

	return res
}

And it did fix the issue as it can be seen there:

https://stackblitz.com/edit/github-9i57jp-wrcype

Again, apologies for the lack of a good and simple reproducible example. I'll try to update this issue as soon as I have found one.

@randall77
Copy link
Contributor

Looks like an invalid value coming back from internal/bytealg/indexbyte_wasm.s:memchr?
@neelance @golang/wasm

@Nemikolh
Copy link
Author

Nemikolh commented Feb 7, 2024

Possibly! I haven't tried to modify indexbyte_wasm.s yet. I suppose that I could modify it to add a call instruction to the gojs debug import?

@Zxilly
Copy link
Member

Zxilly commented Jul 12, 2024

I have a reproducible example for this bug. Please take a look at #68395

@seankhliao seankhliao changed the title (js/wasm): bytes.IndexByte can return -4294967295 when memory usage is above 2^31 bytes: IndexByte can return -4294967295 when memory usage is above 2^31 on js/wasm Jul 12, 2024
@dmitshur dmitshur added this to the Backlog milestone Jul 12, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/597955 mentions this issue: internal/bytealg: extend memchr result correctly

@randall77
Copy link
Contributor

@gopherbot please open backport issues.

This bug causes {bytes,string}.Index functions to return wrong results when used with very large arguments.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #68474 (for 1.21), #68475 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@randall77
Copy link
Contributor

Here's a simple reproducer.

package main

import "bytes"

func main() {
	b := make([]byte, 1<<31+1)
	b[1<<31] = 1
	i := bytes.IndexByte(b, 1)
	if i != 1<<31 {
		println(i)
		panic("bad index")
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly issues NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants