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.

Sign up for GitHub

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

Jump to bottom

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

Open
bcmills opened this issue Sep 26, 2023 · 7 comments
Open

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

bcmills opened this issue Sep 26, 2023 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Unfortunate
Milestone
Proposal

Comments

@bcmills
Copy link
Contributor

bcmills commented Sep 26, 2023 •
edited

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

https://pkg.go.dev/golang.org/x/sys@v0.12.0/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 gopherbot added this to the Unreleased milestone Sep 26, 2023
@gopherbot
Copy link

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

@thanm thanm added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Sep 26, 2023

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
Copy link

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

@qmuntal
Copy link
Contributor

qmuntal commented Sep 26, 2023

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 pushed a commit to golang/sys that referenced this issue Sep 26, 2023
@gopherbot
windows: convert TestCommandLineRecomposition to a fuzz test and fix …
e649453 
…discrepancies

Notably, this fixes the escaping of the first argument when it
contains quoted spaces, and fixes a panic in DecomposeCommandLine
when it contains more than 8192 arguments.

Fixes golang/go#58817.
For golang/go#17149.
For golang/go#63236.

Change-Id: Ib72913b8182998adc1420d73ee0f9dc017dfbf32
Reviewed-on: https://go-review.googlesource.com/c/sys/+/530275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
gopherbot pushed a commit to golang/sys that referenced this issue Sep 26, 2023
@gopherbot
windows: document the return type mismatch for CommandLineToArgv
01c413d 
For golang/go#63236.

Change-Id: Id6c458e2ee2291e28685d24e86c05702d9fd132a
Cq-Include-Trybots: luci.golang.try:x_sys-gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/sys/+/531175
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

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

@bcmills bcmills changed the title x/sys/windows: CommandLineToArgv hard-codes an inappropriate return type syscall,x/sys/windows: CommandLineToArgv hard-codes an inappropriate return type Sep 26, 2023
@bcmills bcmills modified the milestones: Unreleased, Backlog Sep 26, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 26, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Sep 26, 2023

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

@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Sep 26, 2023
gopherbot pushed a commit to golang/sys that referenced this issue Sep 27, 2023
@gopherbot
windows: remove the 8192-codepoint arg limit in FuzzComposeCommandLine
3186bae 
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>
@bcmills bcmills changed the title syscall,x/sys/windows: CommandLineToArgv hard-codes an inappropriate return type proposal: syscall,x/sys/windows: deprecate CommandLineToArgv due to inappropriate return type Sep 28, 2023
@bcmills bcmills modified the milestones: Backlog, Proposal Sep 28, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Sep 28, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows Unfortunate
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
4 participants
@qmuntal @bcmills @gopherbot @thanm