From 748893f139f1f7dc7d21fea6731cb614499a600e Mon Sep 17 00:00:00 2001
From: Anthony Nandaa <profnandaa@gmail.com>
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()`.

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 <profnandaa@gmail.com>
---
 frontend/dockerfile/dockerfile_test.go | 37 ++++++++++++++++++++++++++
 util/system/path.go                    | 23 ++++++++++++++--
 util/system/path_test.go               | 17 ++++++++++++
 3 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go
index 5893d9e021e1..cf9d786f7ce1 100644
--- a/frontend/dockerfile/dockerfile_test.go
+++ b/frontend/dockerfile/dockerfile_test.go
@@ -155,6 +155,7 @@ var allTests = integration.TestFuncs(
 	testNamedMultiplatformInputContext,
 	testNamedFilteredContext,
 	testEmptyDestDir,
+	testPreserveDestDirSlash,
 	testCopyLinkDotDestDir,
 	testCopyLinkEmptyDestDir,
 	testCopyChownCreateDest,
@@ -550,6 +551,42 @@ RUN cmd /V:on /C "set /p tfcontent=<testfile \
 	require.NoError(t, err)
 }
 
+func testPreserveDestDirSlash(t *testing.T, sb integration.Sandbox) {
+	f := getFrontend(t, sb)
+
+	dockerfile := []byte(integration.UnixOrWindows(
+		`
+FROM busybox
+COPY testfile /sample/
+RUN [ "$(cat /sample/testfile)" == "contents0" ]
+`,
+		`
+FROM nanoserver
+COPY testfile /sample/
+RUN cmd /V:on /C "set /p tfcontent=<\sample\testfile \
+	& if !tfcontent! NEQ contents0 (exit 1)"
+`,
+	))
+
+	dir := integration.Tmpdir(
+		t,
+		fstest.CreateFile("Dockerfile", dockerfile, 0600),
+		fstest.CreateFile("testfile", []byte("contents0"), 0600),
+	)
+
+	c, err := client.New(sb.Context(), sb.Address())
+	require.NoError(t, err)
+	defer c.Close()
+
+	_, err = f.Solve(sb.Context(), c, client.SolveOpt{
+		LocalMounts: map[string]fsutil.FS{
+			dockerui.DefaultLocalNameDockerfile: dir,
+			dockerui.DefaultLocalNameContext:    dir,
+		},
+	}, nil)
+	require.NoError(t, err)
+}
+
 func testCopyLinkDotDestDir(t *testing.T, sb integration.Sandbox) {
 	integration.SkipOnPlatform(t, "windows")
 	f := getFrontend(t, sb)
diff --git a/util/system/path.go b/util/system/path.go
index cdfff68007f5..c001ced6cc00 100644
--- a/util/system/path.go
+++ b/util/system/path.go
@@ -193,9 +193,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), nil
 	}
 
 	// We expect all paths to be in C:
@@ -220,5 +221,23 @@ 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), nil
+}
+
+// An adaptation of filepath.Clean to allow an option to
+// retain the trailing slash, on either of the platforms.
+// Returns path with platform specific separators.
+// See https://github.com/moby/buildkit/issues/5249
+func cleanPath(origPath string) string {
+	cleanedPath := filepath.Clean(origPath)
+	// Windows supports both \\ and / as path separator.
+	// This || check will produce a duplicate predicate on non-Windows
+	// as a trade-off for having a platform-specific hasTrailingSlash func.
+	hasTrailingSlash := strings.HasSuffix(origPath, string(filepath.Separator)) ||
+		strings.HasSuffix(origPath, "/")
+
+	if len(cleanedPath) > 1 && hasTrailingSlash {
+		return cleanedPath + string(filepath.Separator)
+	}
+	return cleanedPath
 }
diff --git a/util/system/path_test.go b/util/system/path_test.go
index c8d97c72d5ed..13ddb2be2100 100644
--- a/util/system/path_test.go
+++ b/util/system/path_test.go
@@ -163,6 +163,23 @@ func TestCheckSystemDriveAndRemoveDriveLetter(t *testing.T) {
 	if _, err = CheckSystemDriveAndRemoveDriveLetter(`\\.\C$\test`, "windows"); err == nil {
 		t.Fatalf("UNC path should fail")
 	}
+
+	// windows-style relative paths on linux
+	if path, err = CheckSystemDriveAndRemoveDriveLetter("\\a\\b\\..\\c\\", "windows"); 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("\\a\\b\\..\\c\\", "linux"); 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")
+	}
 }
 
 // TestNormalizeWorkdir tests NormalizeWorkdir