x/sys/windows: TestCommandLineRecomposition failures

[gopherbot]

#!watchflakes
post <- pkg == "golang.org/x/sys/windows" && test == "TestCommandLineRecomposition"

Issue created automatically to collect these failures.

Example (log):

--- FAIL: TestCommandLineRecomposition (0.06s)
panic: windows: string with NUL passed to StringToUTF16 [recovered]
	panic: windows: string with NUL passed to StringToUTF16

goroutine 45 [running]:
testing.tRunner.func1.2({0x138be20, 0x13f74a8})
	C:/workdir/go/src/testing/testing.go:1529 +0x238
testing.tRunner.func1()
	C:/workdir/go/src/testing/testing.go:1532 +0x39f
panic({0x138be20?, 0x13f74a8?})
	C:/workdir/go/src/runtime/panic.go:912 +0x21f
golang.org/x/sys/windows.StringToUTF16({0xc000157c00?, 0xc000157c00?})
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows.go:80 +0x37
golang.org/x/sys/windows.StringToUTF16Ptr(...)
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows.go:101
golang.org/x/sys/windows.DecomposeCommandLine({0xc000157c00?, 0x31?})
	C:/workdir/gopath/src/golang.org/x/sys/windows/exec_windows.go:103 +0x6a
golang.org/x/sys/windows_test.TestCommandLineRecomposition(0xc00011d6c0)
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows_test.go:609 +0xfa
testing.tRunner(0xc00011d6c0, 0x13c5f50)
	C:/workdir/go/src/testing/testing.go:1579 +0xff
created by testing.(*T).Run in goroutine 1
	C:/workdir/go/src/testing/testing.go:1632 +0x3ad

— watchflakes

[gopherbot]

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "golang.org/x/sys/windows" && test == "TestCommandLineRecomposition"

2023-02-28 18:48 windows-amd64-longtest sys@a6bfb89d go@70efe9f7 x/sys/windows.TestCommandLineRecomposition (log)

--- FAIL: TestCommandLineRecomposition (0.06s)
panic: windows: string with NUL passed to StringToUTF16 [recovered]
	panic: windows: string with NUL passed to StringToUTF16

goroutine 45 [running]:
testing.tRunner.func1.2({0x138be20, 0x13f74a8})
	C:/workdir/go/src/testing/testing.go:1529 +0x238
testing.tRunner.func1()
	C:/workdir/go/src/testing/testing.go:1532 +0x39f
panic({0x138be20?, 0x13f74a8?})
	C:/workdir/go/src/runtime/panic.go:912 +0x21f
golang.org/x/sys/windows.StringToUTF16({0xc000157c00?, 0xc000157c00?})
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows.go:80 +0x37
golang.org/x/sys/windows.StringToUTF16Ptr(...)
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows.go:101
golang.org/x/sys/windows.DecomposeCommandLine({0xc000157c00?, 0x31?})
	C:/workdir/gopath/src/golang.org/x/sys/windows/exec_windows.go:103 +0x6a
golang.org/x/sys/windows_test.TestCommandLineRecomposition(0xc00011d6c0)
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows_test.go:609 +0xfa
testing.tRunner(0xc00011d6c0, 0x13c5f50)
	C:/workdir/go/src/testing/testing.go:1579 +0xff
created by testing.(*T).Run in goroutine 1
	C:/workdir/go/src/testing/testing.go:1632 +0x3ad

— watchflakes

[bcmills]

...intriguing. This is a randomized test added in CL 319229 (attn @zx2c4 @alexbrainman).

Looks like it uncovered a bug.

(On a side note, perhaps it should be converted to a fuzz test?)

(CC @golang/windows)

[alexbrainman]

Looks like it uncovered a bug.

Agree.

Also note that we need to fix both copies of syscall.StringToUTF16 and golang.org/x/sys/windows.StringToUTF16 code.

Alex

[alexbrainman]

Also note that we need to fix both copies of syscall.StringToUTF16 and golang.org/x/sys/windows.StringToUTF16 code.

golang.org/x/sys/windows.UTF16FromString appears to call syscall.UTF16FromString (see https://github.com/golang/sys/blob/494aa493ccb0c797af52f75ff7839a241bb26cdd/windows/syscall_windows.go#L89 ), so perhaps we only need to fix syscall versions of these functions.

Alex

[mknyszek]

In triage now, @alexbrainman do you plan to look into this?

[alexbrainman]

@alexbrainman do you plan to look into this?

I briefly looked at the code. And the only way for syscall.StringToUTF16 to return error is that call to bytealg.IndexByteString returns unexpected result:

go/src/syscall/syscall_windows.go

Lines 41 to 45 in 53c91f9

	func UTF16FromString(s string) ([]uint16, error) {
	if bytealg.IndexByteString(s, 0) != -1 {
	return nil, EINVAL
	}
	// In the worst case all characters require two uint16.

I did not look at bytealg.IndexByteString because it is written in assembler. bytealg.IndexByteString replaced simple loop in 489102d . I hope bytealg.IndexByteString and replaced loop are the same. I also noticed that bytealg.IndexByteString does not have any tests.

I cannot reproduce the failure by simply running the test. I am not sure what else I can try.

So leaving this issue for you @mknyszek to work on. Sorry but I don't have much free time to spend here.

Alex

[ianlancetaylor]

The test uses randomized input, and is entirely capable of randomly inserting a NUL byte into the string, which will cause the test to fail. Or so it seems to me.

CC @zx2c4 who added the test in CL 319229.

[alexbrainman]

The test uses randomized input, and is entirely capable of randomly inserting a NUL byte into the string, which will cause the test to fail.

Good point @ianlancetaylor. I completely missed the fact that syscall.StringToUTF16 does not accept strings with NUL bytes inside.

I think we should adjust golang.org/x/sys/windows.DecomposeCommandLine code to also return error for strings with NUL inside. I don't see a situation where DecomposeCommandLine users will want to use strings with NUL byte inside. If someone reasonably complains about this behaviour, we can adjust the code again later. I also propose that we change tests accordingly too.

What do you think?

Alex