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

tonistiigi · 2024-09-12T18:09:00Z

util/system/path.go

 	// Path does not have a drive letter. Just return it.
 	if len(parts) < 2 {
-		return ToSlash(filepath.Clean(path), inputOS), nil
+		return ToSlash(filepath.Clean(path), inputOS) + keepTrailingSlash(parts[0], inputOS), nil


filepath.Clean is still current system dependent here.

Let me rethink the whole of this part, to a more simplified approach.

LMK what you think about this new update. I've made sure that the trailing slash is only kept in the form that it was in the original path \\ or /; and since filepath.Clean() is system dependent, it's only appended if it has not been cleaned up in the first place.

On Linux vs. Windows for instance:

p := "\\sample\\" // on linux fmt.Printf("%v\n", p == filepath.Clean(p)) // true // on windows fmt.Printf("%v\n", p == filepath.Clean(p)) // false

@tonistiigi -- what do you think about handling this for WCOW scenarios only for now? So I made platform specific implementations of keepTrailingSlash.

Why is this issue not appearing in linux? Isn't the issue in https://github.com/moby/buildkit/blob/v0.16.0/util/system/path.go#L75-L76 that is currently linux-only and doesn't handle inputOS .

Thanks for the catch, yes, the issue also silently appears in Linux cross-builds. Silently coz there's no RUN to assertain the failure.

Repro dockerfile without RUN:

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

Cross-build:

$ 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:

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

Fixing for Linux too.

And as for this:

Isn't the issue in https://github.com/moby/buildkit/blob/v0.16.0/util/system/path.go#L75-L76 that is currently linux-only and doesn't handle inputOS .

by the time we get to calling NormalizePath, origPath will already be getting a stripped out /, so there's nothing to keep.

> github.com/moby/buildkit/util/system.NormalizePath() /home/nandaa/dev/oss/buildkit/util/system/path.go:40 (PC: 0x1cfeb6d) 35: func NormalizePath(parent, newPath, inputOS string, keepSlash bool) (string, error) { 36: if inputOS == "" { 37: inputOS = "linux" 38: } 39: => 40: newPath = ToSlash(newPath, inputOS) 41: parent = ToSlash(parent, inputOS) 42: origPath := newPath 43: 44: if parent == "" { 45: parent = "/" (dlv) p newPath "/sample"

It is stripped out at CheckSystemDriveAndRemoveDriveLetter a few steps before, L1787:

So, NormalizePath expected that there will be a trailing slash, hence the keepSlash bool, but it ends up being stripped away before we call into it.

Now fixed for both WCOW and cross-building from Linux, PTAL.

profnandaa · 2024-09-25T05:44:50Z

CI failure seems unrelated?

=== FAIL: client TestClientGatewayIntegration/TestClientGatewayContainerHostNetworkingValidation/worker=containerd/netmode=host (0.97s)
    build_test.go:2133: 
        	Error Trace:	/src/client/build_test.go:2133
        	            				/src/client/build_test.go:2031
        	            				/src/util/testutil/integration/run.go:97
        	            				/src/util/testutil/integration/run.go:211
        	Error:      	Received unexpected error:
        	            	expecting started message, got *moby_buildkit_v1_frontend.ExecMessage_Exit

gabriel-samfira · 2024-09-26T12:46:43Z

util/system/path.go

+// and keeps it. See https://github.com/moby/buildkit/issues/5249
+// Expects cleanedPath to have gone through filepath.Clean()
+// which returns the path with platform specific separators.
+func keepTrailingSlash(cleanedPath, origPath, inputOS string) string {


Nit: Would it make sense to create a CleanPath() function in this package that incorporates keepTrailingSlash()? We could then replace calls to filepath.Clean() with CleanPath()

Perhaps something like: https://go.dev/play/p/lcvDPMnT8xS ? What do you think?

I like that, looks neat, let me do that. Thanks!

Perhaps the only modification I'll do is to make our CleanPath take an additional keepSlash bool param, to be consistent with NormalizePath; as much as rn, it's always true.

So, I've given it a little of some thought, I think we can minimize the surface area to just focus on our adaptation of filepath.Clean to retain the trailing slash. If you don't need to retain the trailing slash, then you just call filepath.Clean. I think this will improve composability and avoid any future surprises.

I have also dropped the inputOS param since the initial intention for it was for the ToSlash function. So we clean and pass it to ToSlash as before.

As for exporting it, I'm not entirely sure. Can wait until the time it will be needed outside?

PTAL.

here's the side-by-side - https://go.dev/play/p/UbsFcgnqvA3

Here are the two calls I'm investigating:

$ grep -rn "\.CheckSystemDriveAndRemoveDriveLetter(" solver/llbsolver/file/backend.go:381: s, err := system.CheckSystemDriveAndRemoveDriveLetter(s, runtime.GOOS) frontend/dockerfile/dockerfile2llb/convert.go:1517: f, err := system.CheckSystemDriveAndRemoveDriveLetter(src.Path, d.platform.OS)

I have run both, hard-coding (swapping linux with windows) and the runs are successful for both permutations.

Results:

#6 [2/2] COPY test1.txt /sample/ #6 DONE 0.1s #7 exporting to client directory #7 copying files 19.71MB 0.1s #7 copying files 230.88MB 0.9s done #7 DONE 0.9s $ ls output/sample test1.txt

So, it's all good.

For completeness, also did the reverse on Windows side (swapping windows with linux) and it's successful:

PS> go test -v --run=TestIntegration/TestPreserveDestDirSlash/worker=containerd <snip> === NAME TestIntegration run.go:431: Skipped on windows --- SKIP: TestIntegration (34.52s) --- PASS: TestIntegration/TestPreserveDestDirSlash/worker=containerd/frontend=builtin (19.11s) --- PASS: TestIntegration/TestPreserveDestDirSlash/worker=containerd/frontend=client (15.34s) PASS ok github.com/moby/buildkit/frontend/dockerfile 34.704s

buildkit can build Windows images on Linux. It uses the inputOS to know the target OS. So when buildkitd runs on Linux and is adding a file inside a Windows image, this code will break if the path is a Windows style path of the form pth4 := "\\a\\b\\..\\c\\"

Please see the last examples from here: https://go.dev/play/p/ZnAPJ2__nis

This is the reason I added the call to cleaned := ToSlash(origPath, inputOS) in my previous example.

I was testing the cross-build case from Linux. Here's using the example path:

FROM mcr.microsoft.com/windows/nanoserver:ltsc2022 COPY test1.txt \\a\\b\\..\\c\\

Build command:

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

Build results:

#6 [2/2] COPY test1.txt \a\b\..\c\ #6 DONE 0.1s #7 exporting to client directory #7 copying files 21.05MB 0.1s #7 copying files 230.88MB 0.9s done #7 DONE 0.9s $ ls output/ a License.txt ProgramData Users Windows $ ls output/a/c test1.txt

previous convo here - #5317 (comment)

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()`. 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 <profnandaa@gmail.com>

util/system/path.go

profnandaa · 2024-10-03T07:53:03Z

PTAL again, addressed the comments. @tonistiigi @gabriel-samfira

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

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 2 times, most recently from 54b0fbd to 4056da0 Compare September 25, 2024 04:02

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

profnandaa force-pushed the fix-5249-copy-dir-path branch from 4056da0 to b4437dc Compare September 25, 2024 05:13

gabriel-samfira reviewed Sep 26, 2024

View reviewed changes

profnandaa force-pushed the fix-5249-copy-dir-path branch from b4437dc to 31e2d43 Compare September 26, 2024 15:18

gabriel-samfira reviewed Sep 28, 2024

View reviewed changes

util/system/path.go Show resolved Hide resolved

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

tonistiigi Sep 12, 2024

profnandaa Sep 12, 2024

profnandaa Sep 13, 2024 •

edited

Loading

profnandaa Sep 17, 2024

tonistiigi Sep 19, 2024

profnandaa Sep 24, 2024 •

edited

Loading

profnandaa Sep 24, 2024

profnandaa Sep 24, 2024

profnandaa Sep 24, 2024 •

edited

Loading

profnandaa commented Sep 25, 2024

gabriel-samfira Sep 26, 2024

profnandaa Sep 26, 2024

profnandaa Sep 26, 2024

profnandaa Sep 26, 2024

profnandaa Sep 26, 2024

profnandaa Oct 3, 2024

profnandaa Oct 3, 2024

gabriel-samfira Oct 3, 2024

profnandaa Oct 3, 2024 •

edited

Loading

profnandaa Oct 3, 2024

profnandaa commented Oct 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profnandaa Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profnandaa Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profnandaa Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

profnandaa commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profnandaa Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

profnandaa commented Oct 3, 2024

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

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

profnandaa commented Sep 10, 2024 •

edited

Loading

profnandaa Sep 13, 2024 •

edited

Loading

profnandaa Sep 24, 2024 •

edited

Loading

profnandaa Sep 24, 2024 •

edited

Loading

profnandaa Oct 3, 2024 •

edited

Loading