code review 4548050: syscall : add ProcAttr field to pass an unescaped comma...

syscall : add ProcAttr field to pass an unescaped command line on windows

On windows, the command line is passed as a single null-terminated string. While the automatic parameter escaping done by syscall.StartProcess works fine with most Windows programs, some applications do their own custom parsing of the command line, in which case the automatic escaping becomes harmful.
This CL adds a new extra CmdLine field to syscall.ProcAttr that will be used as the raw/unescaped command line if not empty.
Fixes issue 1849.

[vincent.vanackere, 2011-05-19 12:18:38]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[brainman, 2011-05-19 12:56:50]

On 2011/05/19 12:18:38, vincent.vanackere wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:alex.brainman@gmail.com (cc:
> mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I don't understand what the problem is. Would you, please, create an issue
http://code.google.com/p/go/issues/list to explain.

Thank you.

Alex

[vincent.vanackere, 2011-05-19 14:11:27]

Done at http://code.google.com/p/go/issues/detail?id=1849

The issue is quite simple : command line parsing is the responsability
of each windows program and while the current escaping works ok for
programs using the standard microsoft c runtime it does not work for
cmd.exe and is probably also wrong for most Delphi-compiled exe that
use different conventions (AFAICT they do not handle '\' escaping at
all and two consecutive double quotes are replaced by a single one).

Best regards,

Vincent

On Thu, May 19, 2011 at 2:56 PM,  <alex.brainman@gmail.com> wrote:
> On 2011/05/19 12:18:38, vincent.vanackere wrote:
>>
>> Hello mailto:golang-dev@googlegroups.com,
>
> mailto:alex.brainman@gmail.com (cc:
>>
>> mailto:golang-dev@googlegroups.com),
>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>
> I don't understand what the problem is. Would you, please, create an
> issue http://code.google.com/p/go/issues/list to explain.
>
> Thank you.
>
> Alex
>
> http://codereview.appspot.com/4548050/
>

[vincent.vanackere, 2011-05-24 21:36:28]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[brainman, 2011-05-30 02:21:13]

Let's see if we can get this moving.

Alex

http://codereview.appspot.com/4548050/diff/1003/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4548050/diff/1003/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:37: }
I think this is too "hacky".

Can we change ProcAttr to have new extra CmdLine field instead

type ProcAttr struct {
        Dir        string
        Env        []string
        Files      []int
        HideWindow bool
        CmdLine    string
}

to pass whatever command line you want there. This should cover for any
eventualities in the future.

I'm also happy to make escape function exported to assist building CmdLine. Just
call it EscapeArgs or something.

[vincent.vanackere, 2011-05-30 09:50:55]

http://codereview.appspot.com/4548050/diff/1003/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4548050/diff/1003/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:37: }
On 2011/05/30 02:21:13, brainman wrote:
> I think this is too "hacky".
> 
> Can we change ProcAttr to have new extra CmdLine field instead
> 
> type ProcAttr struct {
>         Dir        string
>         Env        []string
>         Files      []int
>         HideWindow bool
>         CmdLine    string
> }
> 
> to pass whatever command line you want there. This should cover for any
> eventualities in the future.
> 
> I'm also happy to make escape function exported to assist building CmdLine.
Just
> call it EscapeArgs or something.

Done and done (called it EscapeArg because it escapes a single argument...
unless you meant to export makeCmdLine ?)

[vincent.vanackere, 2011-05-30 11:07:26]

Hello golang-dev@googlegroups.com, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-05-30 16:47:50]

This CL I'm cool with.  I just don't want syscall internals to bleed up into
upper levels.

http://codereview.appspot.com/4548050/diff/7003/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4548050/diff/7003/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:225: CmdLine    string
Package syscall generally doesn't have docs, but you might want to add a little
one here:

CmdLine string // used if non-empty, else blah blah blah

[brainman, 2011-05-31 02:02:01]

LGTM

[vincent.vanackere, 2011-05-31 08:11:56]

PTAL

http://codereview.appspot.com/4548050/diff/7003/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4548050/diff/7003/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:225: CmdLine    string
On 2011/05/30 16:47:50, bradfitz wrote:
> Package syscall generally doesn't have docs, but you might want to add a
little
> one here:
> 
> CmdLine string // used if non-empty, else blah blah blah

Done.

[rsc, 2011-05-31 14:21:32]

LGTM

[rsc, 2011-05-31 14:22:46]

*** Submitted as http://code.google.com/p/go/source/detail?r=c1e1c8b31a8b ***