fix: util/path: `CheckSystemDriveAndRemoveDriveLetter` to preserve `/` #5317

profnandaa · 2024-09-10T08:08:03Z

The call to CheckSystemDriveAndRemoveDriveLetter() does not preserve the trailing / or \\. This happens because filepath.Clean() strips away any trailing slashes. For example /sample/ will be \\sample on Windows. This function was mainly written for Windows scenarios, which have System Drive Letters like C:/, etc.

This was causing cases like COPY testfile /testdir/ to be intepreted as COPY testfile /testdir, and if testdir is not explictly created before the call, it ends up being treated as a destination file other than a directory.

Fix this by checking that if we have a trailing / or \\, we preserve it after the call to filepath.Clean().

Fixes #5249

PS.

Also fixed for cross-building Windows from Linux, that would fail silently:

Repro dockerfile without RUN:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
COPY test1.txt /sample/
#RUN type \sample\test1.txt

Build log:

$ docker buildx build --platform windows/amd64 `
    --builder buildkitd-dev --no-cache --tag=windows-test . `
    --progress plain `
    --output type=local,dest=./output

#6 [2/2] COPY test1.txt /sample/
#6 DONE 0.1s

#7 exporting to client directory
#7 copying files 31B
#7 copying files 230.88MB 0.8s done
#7 DONE 0.8s

Checking results, sample is a file instead of a directory:

# before
$ ls -l output/sample
-rw-r--r-- 1 root root 6 Sep 24 08:57 output/sample

# after
$ ls -l output/sample
total 4
-rw-r--r-- 1 root root 6 Sep 24 08:57 test1.txt

NOTE: also covered cases like, where platform-specific filepath.Clean() won't strip out the \\ on Linux:

COPY test1.txt \\sample\\

profnandaa · 2024-09-10T08:21:16Z

Looking into the CI failures...

profnandaa · 2024-09-11T05:24:36Z

Found the regression caused by this, thankfully coz of the checked in integration tests!

Dockerfile:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022
COPY test1.txt /sample/
RUN type \sample\test1.txt

COPY test1.txt /
COPY test1.txt /test2.txt

RUN type test1.txt
RUN type test2.txt

Build log:

Dockerfile:5
--------------------
   3 |     RUN type \sample\test1.txt
   4 |     
   5 | >>> COPY test1.txt /
   6 |     COPY test1.txt /test2.txt
   7 |     
--------------------
error: failed to solve: removing drive letter: UNC paths are not supported
Process 5812 has exited with status 1

Fixing.

util/system/path.go

gabriel-samfira · 2024-10-09T09:27:44Z

LGTM. Waiting for a second review before merging.

profnandaa · 2024-10-09T10:12:13Z

Thanks!

…

On Wed, Oct 9, 2024, 12:28 Gabriel ***@***.***> wrote: LGTM. Waiting for a second review before merging. — Reply to this email directly, view it on GitHub <#5317 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB7ZEMYKEKKZE5GDTAXVQLZ2TZKPAVCNFSM6AAAAABN6FHHEWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBRHAYDSOBYGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

profnandaa · 2024-10-10T05:40:14Z

@crazy-max @tonistiigi -- can take a look at this one?

tonistiigi

Still don't understand this in relation to #5317 (comment) comment.

We have a NormalizePath function that takes keepSlash parameter. Afaics that implementation is still linux-specific and does not understand windows inputOS with backslashes.

This adds a new function cleanPath() that doesn't take keepSlash but automatically adds / is input path contained it. That is not the behavior of path.Clean so minimally function should be called cleanPathKeepSlash(), but ideally we should keep the functionality to keep slash into a single function and not multiple, and NormalizePath should not be linux-specific.

util/system/path.go

profnandaa · 2024-10-16T07:26:20Z

Hi @tonistiigi --

Still don't understand this in relation to #5317 (comment) comment.

We have a NormalizePath function that takes keepSlash parameter. Afaics that implementation is still linux-specific and does not understand windows inputOS with backslashes.

The problem is that we don't get a chance to get to NormalizePath, the trailing slash is stripped away at CheckSystemDriveAndRemoveDriveLetter. So, there's nothing to keep by that time:

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Lines 1801 to 1822 in 4b4a9aa

    
           func pathRelativeToWorkingDir(s llb.State, p string, platform ocispecs.Platform) (string, error) { 
        
           	dir, err := s.GetDir(context.TODO(), llb.Platform(platform)) 
        
           	if err != nil { 
        
           		return "", err 
        
           	} 
        
           	p, err = system.CheckSystemDriveAndRemoveDriveLetter(p, platform.OS) 
        
           	if err != nil { 
        
           		return "", errors.Wrap(err, "removing drive letter") 
        
           	} 
        
           	if system.IsAbs(p, platform.OS) { 
        
           		return system.NormalizePath("/", p, platform.OS, true) 
        
           	} 
        
           	// add slashes for "" and "." paths 
        
           	// "" is treated as current directory and not necessariy root 
        
           	if p == "." || p == "" { 
        
           		p = "./" 
        
           	} 
        
           	return system.NormalizePath(dir, p, platform.OS, true) 
        
           }

This adds a new function cleanPath() that doesn't take keepSlash but automatically adds / is input path contained it. That is not the behavior of path.Clean so minimally function should be called cleanPathKeepSlash(), but ideally we should keep the functionality to keep slash into a single function and not multiple, and NormalizePath should not be linux-specific.

I'm okay with renaming this for clarity. On the second part, the only problem is that we don't get to the NormalizePath function..
Another idea I'd toiled with is to have add a keepSlash param to CheckSystemDriveAndRemoveDriveLetter; but since this is already an exported function, was going to break the backward compatibility.

The more simplistic option I had is just adding back the trailing slash before we call into NormalizePath on L1821; but felt that will be too patchy...

Your thoughts?

tonistiigi · 2024-10-16T07:34:27Z

but since this is already an exported function

Don't worry about Go backwards compatibility. We only care about API backwards compatibility.

The problem is that we don't get a chance to get to NormalizePath, #5249 (comment). So, there's nothing to keep by that time:

NormalizePath also calls into CheckSystemDriveAndRemoveDriveLetter (2x actually) so it looks like there might be some duplication in the codepath. Better to make it clear which functions responsibility it is to keep the slash and make sure this implementation is cross-platform capable.

profnandaa · 2024-10-16T11:42:08Z

but since this is already an exported function

Don't worry about Go backwards compatibility. We only care about API backwards compatibility.

Thanks for this, we can then have keepSlash (preferably, be declared once and) run through both functions for consistency.

The problem is that we don't get a chance to get to NormalizePath, #5249 (comment). So, there's nothing to keep by that time:

NormalizePath also calls into CheckSystemDriveAndRemoveDriveLetter (2x actually) so it looks like there might be some duplication in the codepath. Better to make it clear which functions responsibility it is to keep the slash and make sure this implementation is cross-platform capable.

I also noticed that multiple runs through CheckSystemDriveAndRemoveDriveLetter; I can work on some optimizations as a follow up.

profnandaa

PTAL at the new changes I've made from the review recommendations. @gabriel-samfira @tonistiigi

util/system/path.go

solver/llbsolver/file/backend.go

profnandaa · 2024-10-17T16:18:19Z

Will investigate and fix the CI failures.

profnandaa · 2024-10-23T07:47:09Z

Will investigate and fix the CI failures.

CI failures now fixed, PTAL @gabriel-samfira @tonistiigi

solver/llbsolver/file/backend.go

util/system/path.go

tonistiigi · 2024-11-09T01:37:00Z

util/system/path.go

+// Returns path with platform specific separators.
+// See https://github.com/moby/buildkit/issues/5249
+func cleanPath(origPath string) string {
+	cleanedPath := filepath.Clean(origPath)


I guess this has not been resolved?

frontend/dockerfile/dockerfile2llb/convert.go

The call to CheckSystemDriveAndRemoveDriveLetter() does not preserve the trailing `/` or `\\`. This happens because `filepath.Clean()` strips away any trailing slashes. For example `/sample/` will be `\\sample` on Windows and `/sample` on Linux. This function was mainly written for Windows scenarios, which have System Drive Letters like C:/, etc. This was causing cases like `COPY testfile /testdir/` to be intepreted as `COPY testfile /testdir`, and if `testdir` is not explictly created before the call, it ends up being treated as a destination file other than a directory. Fix this by checking that if we have a trailing `/` or `\\`, we preserve it after the call to `filepath.Clean()`. Also refactor `CheckSystemDriveAndRemoveDriveLetter` function to take an extra keepSlash bool param, to be consistent with what is passed to `NormalizePath`. The rest of the calls to this function has left keepSlash = false as the default behavior. Fixes moby#5249 PS. Also fixed for cross-building from Linux scenario, taking care for paths like `\\sample\\` that are not changed when run through `filepath.Clean()`. Signed-off-by: Anthony Nandaa <[email protected]>

github-actions bot added area/dockerfile area/testing area/frontend area/util labels Sep 10, 2024

profnandaa requested review from tonistiigi and crazy-max September 10, 2024 08:08

profnandaa changed the title ~~fix: util/path: CheckSystemDriveAndRemoveDriveLetter to preserve /~~ fix: util/path: CheckSystemDriveAndRemoveDriveLetter to preserve / Sep 10, 2024

profnandaa mentioned this pull request Sep 10, 2024

WCOW: trailing / ignored during COPY if destination dir not present #5249

Open

profnandaa force-pushed the fix-5249-copy-dir-path branch 4 times, most recently from ac8552f to af77ee9 Compare September 11, 2024 08:00

tonistiigi reviewed Sep 11, 2024

View reviewed changes

util/system/path.go Outdated Show resolved Hide resolved

profnandaa force-pushed the fix-5249-copy-dir-path branch from af77ee9 to be65686 Compare September 12, 2024 04:24

tonistiigi reviewed Sep 12, 2024

View reviewed changes

util/system/path.go Outdated Show resolved Hide resolved

util/system/path.go Outdated Show resolved Hide resolved

util/system/path.go Outdated Show resolved Hide resolved

profnandaa force-pushed the fix-5249-copy-dir-path branch 3 times, most recently from 36e398e to 1a5289c Compare September 13, 2024 08:21

profnandaa marked this pull request as draft September 13, 2024 08:51

profnandaa force-pushed the fix-5249-copy-dir-path branch from 1a5289c to c55dbc3 Compare September 17, 2024 09:19

github-actions bot added the area/windows label Sep 17, 2024

profnandaa force-pushed the fix-5249-copy-dir-path branch from c55dbc3 to 288b5db Compare September 17, 2024 09:19

profnandaa marked this pull request as ready for review September 17, 2024 17:25

profnandaa force-pushed the fix-5249-copy-dir-path branch from 288b5db to c91001c Compare September 24, 2024 07:46

github-actions bot removed the area/windows label Sep 24, 2024

profnandaa force-pushed the fix-5249-copy-dir-path branch 2 times, most recently from aca0ccb to e8ae74c Compare September 24, 2024 08:27

github-actions bot added the area/windows label Sep 24, 2024

profnandaa force-pushed the fix-5249-copy-dir-path branch 5 times, most recently from 5dd50bb to 2d75bcd Compare October 9, 2024 04:31

gabriel-samfira reviewed Oct 9, 2024

View reviewed changes

util/system/path.go Outdated Show resolved Hide resolved

profnandaa force-pushed the fix-5249-copy-dir-path branch from 2d75bcd to 1dc5d2b Compare October 9, 2024 09:11

gabriel-samfira approved these changes Oct 9, 2024

View reviewed changes

tonistiigi reviewed Oct 15, 2024

View reviewed changes

util/system/path.go Outdated Show resolved Hide resolved

profnandaa force-pushed the fix-5249-copy-dir-path branch from 1dc5d2b to d750530 Compare October 17, 2024 04:59

github-actions bot added the area/solver label Oct 17, 2024

profnandaa commented Oct 17, 2024

View reviewed changes

util/system/path.go Outdated Show resolved Hide resolved

solver/llbsolver/file/backend.go Show resolved Hide resolved

profnandaa force-pushed the fix-5249-copy-dir-path branch from d750530 to 3410347 Compare October 17, 2024 06:04

profnandaa force-pushed the fix-5249-copy-dir-path branch 6 times, most recently from be12cf3 to b8d27d1 Compare October 23, 2024 04:16

tonistiigi reviewed Nov 9, 2024

View reviewed changes

profnandaa force-pushed the fix-5249-copy-dir-path branch from b8d27d1 to 5d4b38c Compare November 9, 2024 03:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: util/path: `CheckSystemDriveAndRemoveDriveLetter` to preserve `/` #5317

fix: util/path: `CheckSystemDriveAndRemoveDriveLetter` to preserve `/` #5317

profnandaa commented Sep 10, 2024 •

edited

Loading

profnandaa commented Sep 10, 2024

profnandaa commented Sep 11, 2024

gabriel-samfira commented Oct 9, 2024

profnandaa commented Oct 9, 2024 via email

profnandaa commented Oct 10, 2024

tonistiigi left a comment •

edited

Loading

profnandaa commented Oct 16, 2024 •

edited

Loading

tonistiigi commented Oct 16, 2024

profnandaa commented Oct 16, 2024

profnandaa left a comment

profnandaa commented Oct 17, 2024

profnandaa commented Oct 23, 2024

tonistiigi Nov 9, 2024

fix: util/path: CheckSystemDriveAndRemoveDriveLetter to preserve / #5317

Are you sure you want to change the base?

fix: util/path: CheckSystemDriveAndRemoveDriveLetter to preserve / #5317

Conversation

profnandaa commented Sep 10, 2024 • edited Loading

PS.

profnandaa commented Sep 10, 2024

profnandaa commented Sep 11, 2024

gabriel-samfira commented Oct 9, 2024

profnandaa commented Oct 9, 2024 via email

profnandaa commented Oct 10, 2024

tonistiigi left a comment • edited Loading

Choose a reason for hiding this comment

profnandaa commented Oct 16, 2024 • edited Loading

tonistiigi commented Oct 16, 2024

profnandaa commented Oct 16, 2024

profnandaa left a comment

Choose a reason for hiding this comment

profnandaa commented Oct 17, 2024

profnandaa commented Oct 23, 2024

tonistiigi Nov 9, 2024

Choose a reason for hiding this comment

fix: util/path: `CheckSystemDriveAndRemoveDriveLetter` to preserve `/` #5317

fix: util/path: `CheckSystemDriveAndRemoveDriveLetter` to preserve `/` #5317

profnandaa commented Sep 10, 2024 •

edited

Loading

tonistiigi left a comment •

edited

Loading

profnandaa commented Oct 16, 2024 •

edited

Loading