x/sys/windows: TestCommandLineRecomposition failures #58817

gopherbot · 2023-03-01T20:38:24Z

#!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

The text was updated successfully, but these errors were encountered:

gopherbot · 2023-03-01T20:38:26Z

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 · 2023-03-01T21:27:31Z

...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 · 2023-03-08T00:01:56Z

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 · 2023-03-08T00:08:28Z

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 · 2023-03-08T20:26:14Z

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

alexbrainman · 2023-03-10T22:24:49Z

@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 · 2023-03-10T23:30:32Z

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 · 2023-03-13T04:33:12Z

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

bcmills · 2023-03-13T21:31:54Z

DecomposeCommandLine is documented to use CommandLineToArgv, and the CommandLineToArgvW system call inherently does not support strings with internal NUL bytes, I agree that it is reasonable for DecomposeCommandLine to reject those strings with an error.

gopherbot · 2023-04-18T17:27:16Z

Found new dashboard test flakes for:

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

2023-04-18 17:08 windows-amd64-race sys@2a33a30b go@ecf7e00d x/sys/windows.TestCommandLineRecomposition (log)

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

goroutine 11 [running]:
testing.tRunner.func1.2({0x63a580, 0x708670})
	C:/workdir/go/src/testing/testing.go:1526 +0x372
testing.tRunner.func1()
	C:/workdir/go/src/testing/testing.go:1529 +0x650
panic({0x63a580, 0x708670})
	C:/workdir/go/src/runtime/panic.go:890 +0x263
golang.org/x/sys/windows.StringToUTF16({0xc0001e6000, 0xbe8})
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows.go:80 +0x7d
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({0xc0001e6000, 0xbe8})
	C:/workdir/gopath/src/golang.org/x/sys/windows/exec_windows.go:103 +0x8a
golang.org/x/sys/windows_test.TestCommandLineRecomposition(0xc000045860)
	C:/workdir/gopath/src/golang.org/x/sys/windows/syscall_windows_test.go:609 +0x125
testing.tRunner(0xc000045860, 0x67c718)
	C:/workdir/go/src/testing/testing.go:1576 +0x217
created by testing.(*T).Run
	C:/workdir/go/src/testing/testing.go:1629 +0x806

— watchflakes

gopherbot · 2023-04-22T07:29:50Z

Change https://go.dev/cl/487695 mentions this issue: windows: return error if DecomposeCommandLine parameter contains NUL

gopherbot · 2023-09-20T00:19:37Z

Found new dashboard test flakes for:

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

2023-09-15 18:45 windows-arm64-11 sys@c7ff727a go@a56e4969 x/sys/windows.TestCommandLineRecomposition (log)

--- FAIL: TestCommandLineRecomposition (0.18s)
    syscall_windows_test.go:603: Unable to decompose "valid-filename-for-arg0 �����\\\"������� \"� \\\"\" �������������������������� ��\\ \" \" �����\\\"� �\\\"\\���� ����\\\"����������������\\�������� ��������������������������\\������� �������\\\"���������\U0003f05d��� �����������������\\\"��� '���� �\\�������������������� ���������\\�������������������� ������������\\������������� ��������������� �����\\\"��� \"\" ���������������������������������� \"���������������� ������\" �\\������������������ ��� ��������'��\\\"� ����� ������������������������� ������������������������������� ������������������������������� ���\\����� ��� ��\x00��\\�������������������������� \"���� ��������������������������\" ��� \"\" \"\" �����������������������\\\"����" made from [valid-filename-for-arg0 �����"������� � " �������������������������� ��\   �����"� �"\���� ����"����������������\�������� ��������������������������\������� �������"���������𿁝��� �����������������"��� '���� �\�������������������� ���������\�������������������� ������������\������������� ��������������� �����"���  ���������������������������������� ���������������� ������ �\������������������ ��� ��������'��"� ����� ������������������������� ������������������������������� ������������������������������� ���\����� ��� �����\�������������������������� ���� �������������������������� ���   �����������������������"����]: string with NUL passed to DecomposeCommandLine

— watchflakes

gopherbot · 2023-09-21T21:05:13Z

Change https://go.dev/cl/530275 mentions this issue: windows: convert TestCommandLineRecomposition to a fuzz test and fix discrepancies

gopherbot · 2023-09-26T17:52:45Z

Change https://go.dev/cl/531176 mentions this issue: windows: remove the 8192-codepoint arg limit in FuzzComposeCommandLine

It just occurred to me that the observed limit was almost certainly a side effect of the Go wrapper for CommandLineToArgv (golang/go#63236) rather than a behavior of the system call itself. Updates golang/go#63236. Updates golang/go#58817. Change-Id: Icc9db01f201f54a78044d1c48e0883e098cfb5e5 Cq-Include-Trybots: luci.golang.try:x_sys-gotip-windows-amd64-longtest Reviewed-on: https://go-review.googlesource.com/c/sys/+/531176 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Bryan Mills <bcmills@google.com> Reviewed-by: Quim Muntal <quimmuntal@gmail.com> Reviewed-by: Than McIntosh <thanm@google.com>

gopherbot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 1, 2023

gopherbot added this to Test Flakes Mar 1, 2023

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 1, 2023

gopherbot added this to the Unreleased milestone Mar 1, 2023

bcmills added the OS-Windows label Mar 1, 2023

mknyszek added this to Go Compiler / Runtime Mar 8, 2023

mknyszek moved this to Todo in Go Compiler / Runtime Mar 8, 2023

dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 25, 2023

gopherbot closed this as completed in golang/sys@6c52899 Apr 25, 2023

github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Apr 25, 2023

github-project-automation bot moved this to Done in Test Flakes Apr 25, 2023

gopherbot reopened this Sep 20, 2023

github-project-automation bot moved this from Done to In Progress in Go Compiler / Runtime Sep 20, 2023

gopherbot closed this as completed in golang/sys@e649453 Sep 26, 2023

github-project-automation bot moved this from In Progress to Done in Go Compiler / Runtime Sep 26, 2023

mknyszek removed this from Go Compiler / Runtime Oct 25, 2023

golang locked and limited conversation to collaborators Sep 25, 2024

gopherbot added the FrozenDueToAge label Sep 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/sys/windows: TestCommandLineRecomposition failures #58817

x/sys/windows: TestCommandLineRecomposition failures #58817

gopherbot commented Mar 1, 2023

gopherbot commented Mar 1, 2023

bcmills commented Mar 1, 2023

alexbrainman commented Mar 8, 2023

alexbrainman commented Mar 8, 2023

mknyszek commented Mar 8, 2023

alexbrainman commented Mar 10, 2023

ianlancetaylor commented Mar 10, 2023

alexbrainman commented Mar 13, 2023

bcmills commented Mar 13, 2023

gopherbot commented Apr 18, 2023

gopherbot commented Apr 22, 2023

gopherbot commented Sep 20, 2023

gopherbot commented Sep 21, 2023

gopherbot commented Sep 26, 2023

x/sys/windows: TestCommandLineRecomposition failures #58817

x/sys/windows: TestCommandLineRecomposition failures #58817

Comments

gopherbot commented Mar 1, 2023

gopherbot commented Mar 1, 2023

bcmills commented Mar 1, 2023

alexbrainman commented Mar 8, 2023

alexbrainman commented Mar 8, 2023

mknyszek commented Mar 8, 2023

alexbrainman commented Mar 10, 2023

ianlancetaylor commented Mar 10, 2023

alexbrainman commented Mar 13, 2023

bcmills commented Mar 13, 2023

gopherbot commented Apr 18, 2023

gopherbot commented Apr 22, 2023

gopherbot commented Sep 20, 2023

gopherbot commented Sep 21, 2023

gopherbot commented Sep 26, 2023