fix: dockerfile2llb: handle escaping of backslashes correctly

[profnandaa]

shlex in was treating single backslash "\" in string as escape sequence, hence stripping them off. Modify processCmdEnv to double escape \\.

This was in turn affecting the vertex Name, and mostly pronounced on Windows where backslashes are prevalent in path names. For example C:\hello\world\path was being rendered as C:helloworldpath, hence confusing users.

Fixes #5250

Build progress before fix:

    #5 [2/4] RUN echo C:helloworldpath
    #5 1.359 C:\hello\world\path
    #5 DONE 1.7s

    #6 [3/4] RUN echo C:\hello\escaped\path
    #6 1.734 C:\\hello\\escaped\\path
    #6 DONE 2.1s

    #7 [4/4] RUN echo "C:\hello\quoted\path"
    #7 1.765 "C:\hello\quoted\path"
    #7 DONE 2.1s

Build progress after fix:

    #5 [2/4] RUN echo C:\hello\world\path
    #5 1.458 C:\hello\world\path
    #5 DONE 1.8s
    
    #6 [3/4] RUN echo C:\\hello\\escaped\\path
    #6 1.730 C:\\hello\\escaped\\path
    #6 DONE 2.1s
    
    #7 [4/4] RUN echo "C:\hello\quoted\path"
    #7 1.702 "C:\hello\quoted\path"
    #7 DONE 2.1s

You can also see that now paths in the step/vertex names now match those in the log lines right after the name.

[tonistiigi]

Not sure this is correct. Don't we need something windows specific what would detect that if path is windows style then single backslashes should mean \\ instead of being escape characters?

[profnandaa]

Not sure this is correct. Don't we need something windows specific what would detect that if path is windows style then single backslashes should mean \\ instead of being escape characters?

Initially I'd thought of just windows-only variant but there such cases like cross-platform builds that could often have the \\, as much as we do normalize them later on, etc. See comment here.

This only has the effect on the vertex.Name after being stored in the llb.customname key #549.

Also you can see that the log lines currently preserve the backslashes correctly, but the stage name:

    #5 [2/4] RUN echo C:helloworldpath
    #5 1.359 C:\hello\world\path
    ~~~~~~~~~~~^
    #5 DONE 1.7s

[tonistiigi]

So what about this:

» cat Dockerfile
from alpine
env foo=aa\bb
run env && stop

 => ERROR [2/2] RUN env && stop                                                            0.2s
------
 > [2/2] RUN env && stop:
0.114 SHLVL=1
0.114 foo=aabb
0.114 HOME=/root

Is it incorrect or correct. Looks like ProcessWord should assume that the value is inside the string in the case of printing command name?

[profnandaa]

Good catch. AFK, let me take a look at this later today. Thanks!

…

On Fri, Aug 23, 2024, 13:11 Tõnis Tiigi ***@***.***> wrote: So what about this: » cat Dockerfile from alpine env foo=aa\bb run env && stop => ERROR [2/2] RUN env && stop 0.2s ------ > [2/2] RUN env && stop: 0.114 SHLVL=1 0.114 foo=aabb 0.114 HOME=/root Is it incorrect or correct. Looks like ProcessWord should assume that the value is inside the string in this case? — Reply to this email directly, view it on GitHub <#5269 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB7ZEMSZW3H4BQ44JJYO33ZS4DGRAVCNFSM6AAAAABM6SN4RSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBWG43DINRTGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[profnandaa]

Let me revisit this right after my vctn, 9/3.

[profnandaa]

@tonistiigi -- I think the log lines are writing out the bytes as received from the RUN call. Linux already strips off the \ itself:

So what about this:

» cat Dockerfile
from alpine
env foo=aa\bb
run env && stop

 => ERROR [2/2] RUN env && stop                                                            0.2s
------
 > [2/2] RUN env && stop:
0.114 SHLVL=1
0.114 foo=aabb
0.114 HOME=/root

Is it incorrect or correct. Looks like ProcessWord should assume that the value is inside the string in the case of printing command name?

Equivalent running in an alpine container shell:

/ # export foo=aa\bb
/ # env
HOSTNAME=3143e71fab36
SHLVL=1
HOME=/root
foo=aabb <--- \ stripped off.
TERM=xterm
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
PWD=/

This is what is coming back the other way round from the lower layers. My fix was fixing only the stage name display, where backslashes are stripped off before they go down from the dockerfile to LLB...

[profnandaa]

PS:

The log lines (just below the stage name), are raw bytes:

buildkit/util/progress/progressui/printer.go

Line 150 in b60d621

fmt.Fprintf(p.w, "#%d %s", v.index, []byte(l))

Then here is where the stage name is printed, but coming from llb.customname:

buildkit/util/progress/progressui/printer.go

Line 76 in b60d621

fmt.Fprintf(p.w, "#%d %s\n", v.index, v.Name)

buildkit/solver/llbsolver/vertex.go

Lines 42 to 47 in b60d621

	func (v *vertex) Name() string {
	if name, ok := v.options.Description["llb.customname"]; ok {
	return name
	}
	return v.name
	}

[profnandaa]

rebased with the latest master.

[tonistiigi]

Testing current (not this PR) in linux:

from alpine
run echo "C:\hello\escaped\path"
run echo C:\hello\escaped\path

#5 [2/3] RUN echo "C:\hello\escaped\path"
#5 0.167 C:\hello\escaped\path
#5 DONE 0.2s

#6 [3/3] RUN echo C:helloescapedpath
#6 0.153 C:helloescapedpath
#6 DONE 0.2s

and btw:

#escape=`
from alpine
run echo "C:\hello\escaped\path"
run echo C:\hello\escaped\path

#5 [2/3] RUN echo "C:\hello\escaped\path"
#5 0.143 C:\hello\escaped\path
#5 DONE 0.2s

#6 [3/3] RUN echo C:\hello\escaped\path
#6 0.146 C:helloescapedpath
#6 DONE 0.2s

Is the issue that Lex is using \ as escape token in windows when it is not actually the escape token?

[profnandaa]

Is the issue that Lex is using \ as escape token in windows when it is not actually the escape token?

That's true, and not only on Windows alone, but Linux too...
Can see that \ is only printed when you put them within quotes.

[profnandaa]

@tonistiigi -- PTAL again.

[tonistiigi]

@profnandaa Has this PR changed since my last comment #5269 (comment) ?

[profnandaa]

@profnandaa Has this PR changed since my last comment #5269 (comment) ?

Relooking into this again. From your test cases, this is what had caught my eyes:

#6 [3/3] RUN echo C:\hello\escaped\path
#6 0.146 C:helloescapedpath <----- this!
#6 DONE 0.2s

Let me see the best way we can address this.