fix: dockerfile2llb: handle escaping of backslashes correctly #5269

profnandaa · 2024-08-22T17:13:56Z

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 · 2024-08-23T09:59:34Z

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 · 2024-08-23T10:11:30Z

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 · 2024-08-23T11:03:04Z

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 · 2024-08-24T19:59:33Z

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

profnandaa · 2024-09-02T15:38:48Z

@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 · 2024-09-02T15:55:46Z

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 
        
           }

`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 moby#5250 Build progress before fix: ``` moby#5 [2/4] RUN echo C:helloworldpath moby#5 1.359 C:\hello\world\path moby#5 DONE 1.7s moby#6 [3/4] RUN echo C:\hello\escaped\path moby#6 1.734 C:\\hello\\escaped\\path moby#6 DONE 2.1s moby#7 [4/4] RUN echo "C:\hello\quoted\path" moby#7 1.765 "C:\hello\quoted\path" moby#7 DONE 2.1s ``` Build progress after fix: ``` moby#5 [2/4] RUN echo C:\hello\world\path moby#5 1.458 C:\hello\world\path moby#5 DONE 1.8s moby#6 [3/4] RUN echo C:\\hello\\escaped\\path moby#6 1.730 C:\\hello\\escaped\\path moby#6 DONE 2.1s moby#7 [4/4] RUN echo "C:\hello\quoted\path" moby#7 1.702 "C:\hello\quoted\path" moby#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. Signed-off-by: Anthony Nandaa <[email protected]>

profnandaa · 2024-09-09T06:49:16Z

rebased with the latest master.

tonistiigi · 2024-09-11T00:43:11Z

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 · 2024-09-11T03:13:17Z

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.

github-actions bot added area/dockerfile area/frontend labels Aug 22, 2024

profnandaa marked this pull request as ready for review August 22, 2024 17:39

profnandaa force-pushed the fix-shlex-backslash branch from 3c5b01f to 32e6711 Compare August 22, 2024 19:46

github-actions bot added the area/testing label Aug 22, 2024

billywr approved these changes Aug 23, 2024

View reviewed changes

tonistiigi reviewed Aug 23, 2024

View reviewed changes

profnandaa marked this pull request as draft August 24, 2024 20:00

profnandaa marked this pull request as ready for review September 2, 2024 15:39

profnandaa force-pushed the fix-shlex-backslash branch from 32e6711 to 538bdcf Compare September 9, 2024 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: dockerfile2llb: handle escaping of backslashes correctly #5269

fix: dockerfile2llb: handle escaping of backslashes correctly #5269

profnandaa commented Aug 22, 2024 •

edited

Loading

tonistiigi left a comment

profnandaa commented Aug 23, 2024 •

edited

Loading

tonistiigi commented Aug 23, 2024 •

edited

Loading

profnandaa commented Aug 23, 2024 via email

profnandaa commented Aug 24, 2024

profnandaa commented Sep 2, 2024 •

edited

Loading

profnandaa commented Sep 2, 2024

profnandaa commented Sep 9, 2024

tonistiigi commented Sep 11, 2024 •

edited

Loading

profnandaa commented Sep 11, 2024

fix: dockerfile2llb: handle escaping of backslashes correctly #5269

Are you sure you want to change the base?

fix: dockerfile2llb: handle escaping of backslashes correctly #5269

Conversation

profnandaa commented Aug 22, 2024 • edited Loading

tonistiigi left a comment

Choose a reason for hiding this comment

profnandaa commented Aug 23, 2024 • edited Loading

tonistiigi commented Aug 23, 2024 • edited Loading

profnandaa commented Aug 23, 2024 via email

profnandaa commented Aug 24, 2024

profnandaa commented Sep 2, 2024 • edited Loading

profnandaa commented Sep 2, 2024

profnandaa commented Sep 9, 2024

tonistiigi commented Sep 11, 2024 • edited Loading

profnandaa commented Sep 11, 2024

profnandaa commented Aug 22, 2024 •

edited

Loading

profnandaa commented Aug 23, 2024 •

edited

Loading

tonistiigi commented Aug 23, 2024 •

edited

Loading

profnandaa commented Sep 2, 2024 •

edited

Loading

tonistiigi commented Sep 11, 2024 •

edited

Loading