proposal: syscall,x/sys/windows: deprecate CommandLineToArgv due to inappropriate return type

[bcmills]

What version of Go are you using (go version)?

https://pkg.go.dev/golang.org/x/[email protected]/windows#CommandLineToArgv

What did you do?

Fuzz test windows.DecomposeCommandLine, which used to use windows.CommandLineToArgv as follows:

	var argc int32
	argv, err := CommandLineToArgv(&utf16CommandLine[0], &argc)
	if err != nil {
		return nil, err
	}
	defer LocalFree(Handle(unsafe.Pointer(argv)))
	var args []string
	for _, v := range (*argv)[:argc] {
		args = append(args, UTF16ToString((*v)[:]))
	}
	return args, nil

What did you expect to see?

A return type from CommandLineToArgv that is consistent with its argc return value, and matches the signature documented in https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw.

What did you see instead?

Go's CommandLineToArgv wrapper hard-codes the bounds on the return value at 8192 entries of 8192 characters each:

func CommandLineToArgv(cmd *uint16, argc *int32) (argv *[8192]*[8192]uint16, err error)

The hard-coded bound is incorrect, and can cause callers to panic when attempting to index into argv by argc.

(attn @golang/windows)

[gopherbot]

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

[bcmills]

I'm not sure how we should address this. Options include:

1. Document it as unfortunate, but leave the exported function signature as-is since almost nobody uses it directly.
2. Deprecate CommandLineToArgv without replacing it, and document that callers should use DecomposeCommandLine instead.
3. Deprecate CommandLineToArgv and replace it with new function (name TBD) with a more correct signature.
4. Make a breaking change to give the return value of the existing CommandLineToArgv the correct type. Callers can use something like argp := (**uint64)(unsafe.Pointer(argv)) to allow their code to compile regardless of which signature is used.

[gopherbot]

Change https://go.dev/cl/531175 mentions this issue: windows: document the return type mismatch for CommandLineToArgv

[qmuntal]

I'm not sure how we should address this. Options include:

I prefer option 2. Current CommandLineToArgv callers should be warned that that API is wrong, and deprecating it is a good way to achieve that. DecomposeCommandLine should be a better fit in most cases.

I also like that users looking for CommandLineToArgv will be redirected to DecomposeCommandLine, which is more difficult to discover given that one normally attempts to duck-type the Windows API name. On the other hand, if we follow option 3, i.e. exporting CommandLineToArgv2, then users would just stick to that after spending some time making sure they are not calling a different Windows API.

[gopherbot]

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

[bcmills]

This also affects the syscall package, which has the same wrapper:
https://pkg.go.dev/syscall?GOOS=windows#CommandLineToArgv

[bcmills]

Converting to a proposal based on option (2). The proposal is, specifically:

• Deprecate syscall.CommandLineToArgv and windows.CommandLineToArgv, and recommend windows.DecomposeCommandLine as the replacement.