ProcessWords: preserve quotes

[vrothberg]

Preserve single and double quotes in ProcessWords to match the
function description and user intention. Also add some simple
unit tests to catch potential future regressions.

Fixes: #108
Signed-off-by: Valentin Rothberg vrothberg@suse.com

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vrothberg
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: nalind

If they are not already assigned, you can assign the PR to them by writing /assign @nalind in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vrothberg]

@rhatdan PTAL. Need Yoda to have a look at the changes as it smells a bit of some side-effects.

[TomSweeneyRedHat]

/ok-to-test

[TomSweeneyRedHat]

@vrothberg do you have a Dockerfile that you could attach here or in the issue that shows the problem before the fix and now works with the fix? I saw the example Dockerfile line, but would like an easy to use Dockerfile if you've one handy.

[TomSweeneyRedHat]

Quick review looks good to me, but I'm not 100% sure the change won't create unintended consequences elsewhere. I'm pretty sure it won't though. @nalind?

[vrothberg]

Thanks a lot for reviewing, @TomSweeneyRedHat!

The core problem is that single and double quotes get removed instead of being escaped. I found this issue while playing with podman-container-runlabel which allows executing a command specified in an image's label. Let's consider the following minimal example Dockerfile:

FROM alpine:latest
LABEL RUN /usr/bin/foo --string-arg "a string arg"

Building and inspecting the image will show that the double quotes are removed:
"RUN": "/usr/bin/foo --string-arg a string arg"

Removing those quotes is prone to cause errors when executing the command, and it generally transforms user input into something unintended and semantically and syntactically different. Please note that this also affects Docker (Cc @thaJeztah).

The concern I have is that some users started to rely on the current behaviour of removing quotes instead of preserving them.

[thaJeztah]

I wonder if this is a bug, or if the intent was to follow shell quoting rules here (this could be if the same rules are applied to ENV, and people expecting it to be processed more in a "shell-like" matter, i.e.;

echo "beta" gamma 'delta'
beta gamma delta

If not, then this could be a bug, where only leading/trailing quotes should be removed from values, but it's applying that rule incorrectly.

The pitfall there being that there are two syntaxes;

LABEL name value
LABEL name=value name=value

Using this Dockerfile;

FROM scratch

# Sets label 'one'
LABEL one two three four

# Sets label 'five' and 'seven'
LABEL five=six seven=eight

# Sets label 'alpha'
LABEL alpha "beta" gamma 'delta'

# Sets label 'epsilon' and 'eta'
LABEL epsilon='zeta' eta="theta"

# Sets label 'quoted'
LABEL quoted "a quoted \"value\""

# Sets label 'quoted2'
LABEL quoted2="another quoted \"value\""

Building the above currently produces;

{
  "alpha": "beta gamma delta",
  "epsilon": "zeta",
  "eta": "theta",
  "five": "six",
  "one": "two three four",
  "quoted": "a quoted \"value\"",
  "quoted2": "another quoted \"value\"",
  "seven": "eight"
}

But if the intent was to remove leading/trailing quotes, I think the expected results would be:

{
  "alpha": "\"beta\" gamma 'delta'",
  "epsilon": "zeta",
  "eta": "theta",
  "five": "six",
  "one": "two three four",
  "quoted": "a quoted \"value\"",
  "quoted2": "another quoted \"value\"",
  "seven": "eight"
}

[thaJeztah]

ping @tonistiigi @AkihiroSuda @duglin PTAL

[thaJeztah]

I found this earlier discussion around this; moby/moby#36027 (comment) (also moby/moby#32140 (comment)

[thaJeztah]

From those discussions (and the docs change in docker/cli#814), it looks like the intent was to process it with shell rules (and thus quotes to be stripped);

The value will be interpreted for other environment variables, so quote characters will be removed if they are not escape

[vrothberg]

@thaJeztah, thanks a lot for the very valuable input!

Your comments made my mind shift to the question whether the values of LABEL must be processed the same way as the ones of ENV. My personal preference would be to preserve quotes and to not transform the input for labels, but given it's been like that for such a long while I fear there's nothing to change without breaking existing workloads.

For the use case of podman-container-runlabel, we might need to mention this explicitly in the docs to avoid this pitfall.

[thaJeztah]

Perhaps @justincormack's proposal in moby/moby#32140 (comment) would be a solution for this; add a JSON syntax for LABEL (and ENV) that won't substitute variables, e.g.;

LABEL {"RUN":"/usr/bin/foo --string-arg \"a string arg\""}

or

LABEL ["RUN=/usr/bin/foo --string-arg \"a string arg\""]

Pitfalls there are (although less problematic than for the RUN syntax, which is ambiguous), not everyone knows that JSON doesn't support single quotes (see moby/moby#5701, and moby/moby#7998)

[duglin]

The intent was to preserve quotes like the linux shell, however, since the value args all get squashed into one string things get a little weird. Look at this:

$ cat a.sh
echo $*

$ echo ads "asd asd" asd | xargs ./a.sh
ads asd asd asd

Notice no quotes.

I would point out that:
LABEL RUN2 "/usr/bin/foo --string-arg 'a string arg'"
does give you want you're looking for.
"RUN2": "/usr/bin/foo --string-arg 'a string arg'",

So, if you want to keep the quotes you may need to wrap the whole thing with more quotes, or escape things:

For example:
ENV foo3 asd \"asd qwe\" qwe
yields:
"foo3=asd "asd qwe" qwe",

Which does seem to make some sense to me. But I'm not a sh expert to know for certain.

[rhatdan]

I think we do what the user expects. I would expect that LABEL or a RUN command to work the same as if I typed in the command at the shell.

Currently we have the users jumping through hoops to make this work. So I like the idea of this patch.

The issue will be what happens if the programmer did jump through hoops will we break that use case.

Does
ENV foo3 asd "asd qwe" qwe
end up with
"foo3=asd "asd qwe" qwe",

[vrothberg]

Does
ENV foo3 asd "asd qwe" qwe
end up with
"foo3=asd "asd qwe" qwe",

No. It ends put with "foo3=asd asd qwe qwe" (i.e., quotes removed).

[vrothberg]

I think we can really break it down to if LABEL should be processed in a similar fashion as ENV.

ENTRYPOINT, for instance, is processed differently (for the reasons mentioned above) and quotes are properly escaped: "/usr/bin/foo --string-arg \"a string arg\""

[thaJeztah]

The difference there is that ENTRYPOINT is used as argument for /bin/sh -c, and to be evaluated by /bin/sh at runtime, whereas LABEL is processed/evaluated during build.

Given this Dockerfile;

FROM scratch
ENTRYPOINT /usr/bin/foo --string-arg "a string arg"

The entrypoint will be;

docker inspect --format '{{json .Config.Entrypoint}}' myimage
["/bin/sh","-c","/usr/bin/foo --string-arg \"a string arg\""]

[thaJeztah]

Irregardless of the above; changing this would be quite a breaking change, and potentially breaking a lot of existing Dockerfiles

[vrothberg]

Irregardless of the above; changing this would be quite a breaking change, and potentially breaking a lot of existing Dockerfiles

That's what I am afraid of as well.

[vrothberg]

I am closing this PR as I changing this behavior might cause regressions for some users. Quotes need to be escaped manually and we need to educate users to be aware of that.