Fix commandline double quoting for job containers #1207
Conversation
|
Is there a test we could add here? |
Oh oop, this was supposed to be draft. Mark confirmed the commandline looks fine now but he's running tests right now to make sure it works. And, probably? Not sure how best to test this as it was Go doing the mangling. I guess we could have some binary that checks os.Args to make sure there's no wonky quotes in a specific position. |
dcantah
force-pushed
the
jobcontainer-cmdline-fix
branch
from
ed85b3d to
594236e
Compare
|
@katiewasnothere Added a test. Will wait to hear back from Mark on if his experiment worked now. |
We already escape the arguments passed to us by Containerd to form a Windows style commandline, however the commandline was being split back into arguments and then passed to exec.Cmd from the go stdlib. exec.Cmd internally also does escaping, which ended up applying some extra quotes in some cases where the commandline had double/single quotes present. This change just passes the commandline as is to the Cmdline field on the Windows syscall.SysProcAttr. Go takes this field as is and doesn't do any further processing on it which is the behavior we desire. Signed-off-by: Daniel Canter <[email protected]>
This change adds a test to verify that commandlines with quotes don't get additional quotes added on to them when combining the arguments given. Signed-off-by: Daniel Canter <[email protected]>
dcantah
force-pushed
the
jobcontainer-cmdline-fix
branch
from
594236e to
bc5e914
Compare
|
@katiewasnothere Mark confirmed this works, PTAL |
| @@ -0,0 +1,11 @@ | |||
| package main | |||
There was a problem hiding this comment.
I use this to debug arg parsing issues:
package main
import (
"fmt"
"os"
)
func main() {
for i, arg := range os.Args {
fmt.Printf("%d: %s\n", i, arg)
}
}
Find it a little easier to understand with the numbered args.
There was a problem hiding this comment.
Would you prefer we do this here and check a specific position for the test?
There was a problem hiding this comment.
I think it will make it slightly easier to tell what's going on if something breaks
| // from whatever process is going to launch the container. | ||
| CreationFlags: windows.CREATE_NEW_PROCESS_GROUP | windows.CREATE_BREAKAWAY_FROM_JOB, | ||
| Token: syscall.Token(token), | ||
| CmdLine: commandLine, |
There was a problem hiding this comment.
Ashamed I didn't know of this
kevpar
left a comment
kevpar
left a comment
There was a problem hiding this comment.
Little suggestion but LGTM either way
|
@kevpar Thanks for review! I have a note to add some extra job container tests regardless so I think I'll check this in and revisit the arg image layout and maybe add some more tests regarding cmdline. |
Related work items: microsoft#1067, microsoft#1097, microsoft#1119, microsoft#1170, microsoft#1176, microsoft#1180, microsoft#1181, microsoft#1182, microsoft#1183, microsoft#1184, microsoft#1185, microsoft#1186, microsoft#1187, microsoft#1188, microsoft#1189, microsoft#1191, microsoft#1193, microsoft#1194, microsoft#1195, microsoft#1196, microsoft#1197, microsoft#1200, microsoft#1201, microsoft#1202, microsoft#1203, microsoft#1204, microsoft#1205, microsoft#1206, microsoft#1207, microsoft#1209, microsoft#1210, microsoft#1211, microsoft#1218, microsoft#1219, microsoft#1220, microsoft#1223
3 participants
We already escape the arguments passed to us by Containerd to form a
Windows style commandline, however the commandline was being split back
into arguments and then passed to exec.Cmd from the go stdlib. exec.Cmd
internally also does escaping, which ended up applying some extra quotes
in some cases where the commandline had double/single quotes present. This change
just passes the commandline as is to the Cmdline field on the Windows
syscall.SysProcAttr. Go takes this field as is and doesn't do any further
processing on it which is the behavior we desire.