Prevent double escaping of Dockerfile commands on Windows by darstahl · Pull Request #17851 · moby/moby

darstahl · 2015-11-09T23:41:28Z

This fixes #17509

Signed-off-by: Darren Stahl [email protected]

lowenna · 2015-11-09T23:56:49Z

@thaJeztah Any chance of a TP4 tag. This has regressed behaviour from TP3 reported in #17509

thaJeztah · 2015-11-10T10:39:52Z

IIUC, this affects the API, so added the impact-label (so we don't forget), may need a docs update as well

runcom · 2015-11-10T10:50:12Z

runconfig/config.go

Does this really live in Config? Is it needed to be committed to the final image?

This seems also so platform specific

Where is the best place to put it such that it can be sent from dockerfile/dispatchers.go to the execdriver where it needs to be processed? I put it in Config, as that seemed to be the location for similar parameters. And yes, it is platform specific.

On second thought, I think it does need to live in Config, since it is not a runtime configuration, but rather reflecting the state of the Command.

darstahl · 2015-11-12T00:34:03Z

Rebased

lowenna · 2015-11-12T01:55:59Z

@runcom - Hey Antonio - which bit of the design are you concerned about? That we're adding something to the persistent state? Unfortunately, I believe we need this persisted there (rather than in hostConfig) as it applies to the command which is stored in there, nothing to do with the host configuration where the container runs. There's no way of breaking the config (or hostconfig) into platform specific parts as these are passed through the API complete. It's only daemon side that the platform daemon can work out what applies and what doesn't. This isn't new though or without precedent - there are certainly linux-specific fields in Config (for example we don't have a TTY; support the recently introduced StopSignal as signals don't exist on Windows; User; or PublishService among probably others). Can you ping me on IRC when you get a chance? I'm concerned if this one gets held up as it breaks some images which need to get built for TP4, plus fixes a TP3 regression, and time is running desperately out now. Thanks! 👍

@darrenstahlmsft I do think though you should postpend the comment in the added field (Windows specific) though to make it clear.

lowenna · 2015-11-12T02:03:28Z

BTW - @darrenstahlmsft can you add updates to the API docs. I think docs\reference\api\docker_remote_api_v1.22.md is the right place.

darstahl · 2015-11-12T02:06:10Z

@jhowardmsft Will do on both accounts (just updated PR before I saw your second comment)

darstahl · 2015-11-12T02:33:49Z

Docs updated, plus moved ArgsEscaped to right after Cmd, as it is directly related. Should I add an example (or change an existing one) of when ArgsEscaped is true?

lowenna · 2015-11-12T02:36:10Z

It wouldn't hurt to add a new one :)

LK4D4 · 2015-11-12T23:25:34Z

daemon/execdriver/windows/commandlinebuilder.go

I think this will be logged somewhere upper anyway, at least it should

Ah, it was there already. But it shouldn't, also such logline will tell nothing to reader.

darstahl · 2015-11-13T00:58:27Z

Requested changes made, and docs changes removed (Will resend those in a separate PR)

LK4D4 · 2015-11-13T01:50:50Z

daemon/execdriver/windows/commandlinebuilder.go

can be just return "", errors.New(...)

LK4D4 · 2015-11-13T01:52:08Z

LGTM apart from nit

lowenna · 2015-11-13T15:13:09Z

Ping @runcom

…s on Windows Signed-off-by: Darren Stahl <[email protected]>

darstahl · 2015-11-13T18:44:26Z

Nit fixed

runcom · 2015-11-14T16:25:33Z

LGTM

Prevent double escaping of Dockerfile commands on Windows

runcom · 2015-11-14T16:28:16Z

Agreed with @jhowardmsft to have a follow-up PR to address docs (if any are needed) ping @thaJeztah

StefanScherer · 2015-11-14T19:50:19Z

@darrenstahlmsft Thanks! But I have to wait until TP4 as of #17919 😢 ;-) But shouldn't take too long I guess.

GordonTheTurtle added area/distribution Image Distribution platform/windows status/0-triage labels Nov 9, 2015

darstahl force-pushed the 10662-ArgumentEscaping branch from 2c9f026 to 3d365fd Compare November 9, 2015 23:43

vdemeester added this to the Windows TP4 milestone Nov 10, 2015

vdemeester added status/2-code-review and removed status/0-triage labels Nov 10, 2015

thaJeztah added the impact/api label Nov 10, 2015

runcom reviewed Nov 10, 2015
View reviewed changes

runcom added status/1-design-review and removed status/2-code-review labels Nov 10, 2015

darstahl force-pushed the 10662-ArgumentEscaping branch from 3d365fd to f1476a9 Compare November 12, 2015 00:31

darstahl force-pushed the 10662-ArgumentEscaping branch from f1476a9 to bc7ea3f Compare November 12, 2015 02:04

darstahl force-pushed the 10662-ArgumentEscaping branch from bc7ea3f to c9c9a5f Compare November 12, 2015 02:32

LK4D4 reviewed Nov 12, 2015
View reviewed changes

darstahl force-pushed the 10662-ArgumentEscaping branch from c9c9a5f to 8221a6c Compare November 13, 2015 00:57

darstahl force-pushed the 10662-ArgumentEscaping branch from 8221a6c to 51940d4 Compare November 13, 2015 01:13

LK4D4 reviewed Nov 13, 2015
View reviewed changes

daemon/execdriver/windows/commandlinebuilder.go Outdated

Copy link

Contributor

LK4D4 Nov 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be just return "", errors.New(...)

This fixes the case where arguments are escaped twice from Dockerfile…

9db5db1

…s on Windows Signed-off-by: Darren Stahl <[email protected]>

darstahl force-pushed the 10662-ArgumentEscaping branch from 51940d4 to 9db5db1 Compare November 13, 2015 18:43

runcom added status/2-code-review and removed status/1-design-review labels Nov 14, 2015

runcom added a commit that referenced this pull request Nov 14, 2015

Merge pull request #17851 from Microsoft/10662-ArgumentEscaping

ad8a665

Prevent double escaping of Dockerfile commands on Windows

runcom merged commit ad8a665 into moby:master Nov 14, 2015

darstahl mentioned this pull request Nov 16, 2015

Windows: Fix docker exec #18025

Merged

lowenna mentioned this pull request Feb 11, 2016

Add support for running a command in a container #19746

Closed

This was referenced Aug 30, 2023

image/spec: Add Go structs #46363

Merged

image/spec: minor fixes and touch-ups in markdown files #46370

Merged

image/specs: add missing OnBuild (v1.0), ArgsEscaped (v1.1), and Shell (v1.2) fields #46371

Merged

Conversation

darstahl commented Nov 9, 2015

Uh oh!

lowenna commented Nov 9, 2015

Uh oh!

thaJeztah commented Nov 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darstahl commented Nov 12, 2015

Uh oh!

lowenna commented Nov 12, 2015

Uh oh!

lowenna commented Nov 12, 2015

Uh oh!

darstahl commented Nov 12, 2015

Uh oh!

darstahl commented Nov 12, 2015

Uh oh!

lowenna commented Nov 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darstahl commented Nov 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Nov 13, 2015

Uh oh!

lowenna commented Nov 13, 2015

Uh oh!

darstahl commented Nov 13, 2015

Uh oh!

runcom commented Nov 14, 2015

Uh oh!

runcom commented Nov 14, 2015

Uh oh!

StefanScherer commented Nov 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants