Windows: CMD not honouring arg escaping#22868
Conversation
4b1187b to
ad9d476
Compare
|
|
||
| b.runConfig.Cmd = strslice.StrSlice(cmdSlice) | ||
| // set config as already being escaped, this prevents double escaping on windows | ||
| b.runConfig.ArgsEscaped = true |
There was a problem hiding this comment.
Where are the args getting escaped in the JSON case?
There was a problem hiding this comment.
They're not and they're not supposed to be.
There was a problem hiding this comment.
Why aren't they supposed to be? What program are we having trouble running?
There was a problem hiding this comment.
JSON works fine. It's the shell form that has issues. See issue linked to ^^
There was a problem hiding this comment.
I don't think you want to set ArgsEscaped=true in the JSON case
There was a problem hiding this comment.
It does, but it's a different, albeit related issue. Can you file a new issue?
There was a problem hiding this comment.
I don't fully grok this PR, but if you'd adding this for CMD, do you need to add it for ENTRYPOINT?
There was a problem hiding this comment.
Let me look into that. Gut reaction is no, but if it turns out the answer is yes, I can follow on with a separate PR.
There was a problem hiding this comment.
If you don't need it for Entrypoint then I'm really confused because its has args just like Cmd does.
|
Commit title has a misspelling in it. A spurious u seems to have snuck in. |
|
🇬🇧 |
| if len(userConf.Entrypoint) == 0 { | ||
| if len(userConf.Cmd) == 0 { | ||
| userConf.Cmd = imageConf.Cmd | ||
| userConf.ArgsEscaped = imageConf.ArgsEscaped |
There was a problem hiding this comment.
I'm a bit confused by the original purpose behind this flag. It seems that by the time we get to daemon.CreateSpec() if the args are not escaped, we escape them - and this is just for windows. Why aren't we just always escaping the args in the first place (on windows) whenever we stash them in the Cmd and EntryPoint arrays - then we don't need to have this flag at all and we can always just blindly copy the data into the spec. I must be missing something.
There was a problem hiding this comment.
At some point yes, this is the right direction to go in and something I'm actively looking at doing in the future - have a proof of concept of that, but it's non trivial unfortunately.
|
Can we have a test-case for this? |
ad9d476 to
2e0ee95
Compare
|
Added |
2e0ee95 to
1b46809
Compare
There was a problem hiding this comment.
Actually, was wondering about a test that tested what went wrong without the change made in this PR (you mentioned things being double escaping on Windows - shouldn't we test for that?)
|
GH is being slow to update. May 23, 2016 09:53 Pacific Daylight TimeSome users may experience a delay in pushes or other changes appearing on the site. |
281387d to
492d016
Compare
|
retest this please |
4037c22 to
1289fbc
Compare
There was a problem hiding this comment.
Looks like you forgot to remove some debugging code here
|
LGTM after my nit is addressed |
1289fbc to
808bbed
Compare
|
Ooops. Fixed this locally, just forgot to push. Updated. |
|
ping @duglin @tiborvass PTAL |
|
ping @jhowardmsft sorry, looks like this needs a rebase now 😢 |
Signed-off-by: John Howard <jhoward@microsoft.com>
808bbed to
d05d021
Compare
|
Rebased |
|
ping @duglin |
|
While I'd prefer that we do the full/real solution, I'll defer to @jhowardmsft's judgement that it should be done in a separate PR. I'm assuming we'd end up removing this extra config param too, right? LGTM |
|
Yup. That's the plan |
|
looks good to me |
|
looks like we have 2 LGTM's and all green. Merging |
Signed-off-by: John Howard jhoward@microsoft.com
Fixes https://github.com/Microsoft/Virtualization-Documentation/issues/266#issuecomment-220093401 when validated in conjunction with #22489.
@darrenstahlmsft There are two things here: This ensures that the builder also sets the argument escaping for CMD (Windows specific), the exact same way it does for RUN. The second is that when merging an image config with a user config, we were not honouring the fact that arguments had been escaped.