From 09e845f572edcf59047277e40ee66fa194b103d1 Mon Sep 17 00:00:00 2001 From: Anthony Nandaa Date: Tue, 10 Sep 2024 00:55:17 -0700 Subject: [PATCH] fix: util/path: CheckSystemDriveAndRemoveDriveLetter to preserve `/` 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 #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 --- frontend/dockerfile/dockerfile2llb/convert.go | 4 +- frontend/dockerfile/dockerfile_test.go | 37 ++++++++++++ solver/llbsolver/file/backend.go | 38 ++----------- util/system/path.go | 39 ++++++++++--- util/system/path_test.go | 57 +++++++++++++++---- 5 files changed, 123 insertions(+), 52 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 791f4d4a50ed..cae406dac7a3 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -1562,7 +1562,7 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error { commitMessage.WriteString(" <<" + src.Path) data := src.Data - f, err := system.CheckSystemDriveAndRemoveDriveLetter(src.Path, d.platform.OS) + f, err := system.CheckSystemDriveAndRemoveDriveLetter(src.Path, d.platform.OS, false) if err != nil { return errors.Wrap(err, "removing drive letter") } @@ -1830,7 +1830,7 @@ func pathRelativeToWorkingDir(s llb.State, p string, platform ocispecs.Platform) return "", err } - p, err = system.CheckSystemDriveAndRemoveDriveLetter(p, platform.OS) + p, err = system.CheckSystemDriveAndRemoveDriveLetter(p, platform.OS, true) if err != nil { return "", errors.Wrap(err, "removing drive letter") } diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 2e7eba86715b..9f01ce01d71a 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -159,6 +159,7 @@ var allTests = integration.TestFuncs( testNamedMultiplatformInputContext, testNamedFilteredContext, testEmptyDestDir, + testPreserveDestDirSlash, testCopyLinkDotDestDir, testCopyLinkEmptyDestDir, testCopyChownCreateDest, @@ -558,6 +559,42 @@ RUN cmd /V:on /C "set /p tfcontent= Fail -func CheckSystemDriveAndRemoveDriveLetter(path string, inputOS string) (string, error) { +func CheckSystemDriveAndRemoveDriveLetter(path string, inputOS string, keepSlash bool) (string, error) { if inputOS == "" { inputOS = "linux" } @@ -193,9 +192,10 @@ func CheckSystemDriveAndRemoveDriveLetter(path string, inputOS string) (string, } parts := strings.SplitN(path, ":", 2) + // Path does not have a drive letter. Just return it. if len(parts) < 2 { - return ToSlash(filepath.Clean(path), inputOS), nil + return ToSlash(cleanPath(path, inputOS, keepSlash), inputOS), nil } // We expect all paths to be in C: @@ -220,5 +220,30 @@ func CheckSystemDriveAndRemoveDriveLetter(path string, inputOS string) (string, // // We must return the second element of the split path, as is, without attempting to convert // it to an absolute path. We have no knowledge of the CWD; that is treated elsewhere. - return ToSlash(filepath.Clean(parts[1]), inputOS), nil + return ToSlash(cleanPath(parts[1], inputOS, keepSlash), inputOS), nil +} + +// An adaptation of filepath.Clean to allow an option to +// retain the trailing slash, on either of the platforms. +// See https://github.com/moby/buildkit/issues/5249 +func cleanPath(origPath, inputOS string, keepSlash bool) string { + // so as to handle cases like \\a\\b\\..\\c\\ + // on Linux, when inputOS is Windows + origPath = ToSlash(origPath, inputOS) + + if !keepSlash { + return path.Clean(origPath) + } + + cleanedPath := path.Clean(origPath) + // Windows supports both \\ and / as path separator. + hasTrailingSlash := strings.HasSuffix(origPath, "/") + if inputOS == "windows" { + hasTrailingSlash = hasTrailingSlash || strings.HasSuffix(origPath, "\\") + } + + if len(cleanedPath) > 1 && hasTrailingSlash { + return cleanedPath + "/" + } + return cleanedPath } diff --git a/util/system/path_test.go b/util/system/path_test.go index 086c63a34480..da42472b6ca2 100644 --- a/util/system/path_test.go +++ b/util/system/path_test.go @@ -87,8 +87,9 @@ func TestNormalizeWorkdir(t *testing.T) { // TestCheckSystemDriveAndRemoveDriveLetter tests CheckSystemDriveAndRemoveDriveLetter func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { + keepSlash := false // Fails if not C drive. - _, err := CheckSystemDriveAndRemoveDriveLetter(`d:\`, "windows") + _, err := CheckSystemDriveAndRemoveDriveLetter(`d:\`, "windows", keepSlash) if err == nil || err.Error() != "The specified path is not on the system drive (C:)" { t.Fatalf("Expected error for d:") } @@ -96,7 +97,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { var path string // Single character is unchanged - if path, err = CheckSystemDriveAndRemoveDriveLetter("z", "windows"); err != nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter("z", "windows", keepSlash); err != nil { t.Fatalf("Single character should pass") } if path != "z" { @@ -104,7 +105,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // Two characters without colon is unchanged - if path, err = CheckSystemDriveAndRemoveDriveLetter("AB", "windows"); err != nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter("AB", "windows", keepSlash); err != nil { t.Fatalf("2 characters without colon should pass") } if path != "AB" { @@ -112,7 +113,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // Abs path without drive letter - if path, err = CheckSystemDriveAndRemoveDriveLetter(`\l`, "windows"); err != nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter(`\l`, "windows", keepSlash); err != nil { t.Fatalf("abs path no drive letter should pass") } if path != `/l` { @@ -120,7 +121,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // Abs path without drive letter, linux style - if path, err = CheckSystemDriveAndRemoveDriveLetter(`/l`, "windows"); err != nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter(`/l`, "windows", keepSlash); err != nil { t.Fatalf("abs path no drive letter linux style should pass") } if path != `/l` { @@ -128,7 +129,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // Drive-colon should be stripped - if path, err = CheckSystemDriveAndRemoveDriveLetter(`c:\`, "windows"); err != nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter(`c:\`, "windows", keepSlash); err != nil { t.Fatalf("An absolute path should pass") } if path != `/` { @@ -136,7 +137,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // Verify with a linux-style path - if path, err = CheckSystemDriveAndRemoveDriveLetter(`c:/`, "windows"); err != nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter(`c:/`, "windows", keepSlash); err != nil { t.Fatalf("An absolute path should pass") } if path != `/` { @@ -144,7 +145,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // Failure on c: - if path, err = CheckSystemDriveAndRemoveDriveLetter(`c:`, "windows"); err == nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter(`c:`, "windows", keepSlash); err == nil { t.Fatalf("c: should fail") } if err.Error() != `No relative path specified in "c:"` { @@ -152,7 +153,7 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // Failure on d: - if path, err = CheckSystemDriveAndRemoveDriveLetter(`d:`, "windows"); err == nil { + if path, err = CheckSystemDriveAndRemoveDriveLetter(`d:`, "windows", keepSlash); err == nil { t.Fatalf("c: should fail") } if err.Error() != `No relative path specified in "d:"` { @@ -160,9 +161,45 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) { } // UNC path should fail. - if _, err = CheckSystemDriveAndRemoveDriveLetter(`\\.\C$\test`, "windows"); err == nil { + if _, err = CheckSystemDriveAndRemoveDriveLetter(`\\.\C$\test`, "windows", keepSlash); err == nil { t.Fatalf("UNC path should fail") } + + // also testing for keepSlash = true + keepSlash = true + origPath := "\\a\\b\\..\\c\\" + if path, err = CheckSystemDriveAndRemoveDriveLetter(origPath, "windows", keepSlash); err != nil { + t.Fatalf("windows relative paths should be cleaned and should pass") + } + // When input OS is Windows, the path should be properly cleaned + if path != "/a/c/" { + t.Fatalf("Path was not cleaned successfully") + } + + if path, err = CheckSystemDriveAndRemoveDriveLetter(origPath, "windows", false); err != nil { + t.Fatalf("windows relative paths should be cleaned and should pass [keepSlash = false]") + } + // When input OS is Windows, the path should be properly cleaned + if path != "/a/c" { + t.Fatalf("Path was not cleaned successfully [keepSlash = false]") + } + + // windows-style relative paths on linux + if path, err = CheckSystemDriveAndRemoveDriveLetter(origPath, "linux", keepSlash); err != nil { + t.Fatalf("windows style relative paths should be considered a valid path element in linux and should pass") + } + // When input OS is Linux, this is a valid path element name. + if path != "\\a\\b\\..\\c\\" { + t.Fatalf("Path was not cleaned successfully") + } + + if path, err = CheckSystemDriveAndRemoveDriveLetter(origPath, "linux", false); err != nil { + t.Fatalf("windows style relative paths should be considered a valid path element in linux and should pass") + } + // When input OS is Linux, this is a valid path element name. + if path != "\\a\\b\\..\\c\\" { + t.Fatalf("Path was not cleaned successfully [keepSlash = false]") + } } // TestNormalizeWorkdirWindows tests NormalizeWorkdir