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

frontend/dockerfile/dockerfile2llb/convert.go

@@ -1536,7 +1536,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")
 		}
@@ -1804,21 +1804,23 @@ func pathRelativeToWorkingDir(s llb.State, p string, platform ocispecs.Platform)
 		return "", err
 	}
 
-	p, err = system.CheckSystemDriveAndRemoveDriveLetter(p, platform.OS)
+	keepSlash := true
+
+	p, err = system.CheckSystemDriveAndRemoveDriveLetter(p, platform.OS, keepSlash)
 	if err != nil {
 		return "", errors.Wrap(err, "removing drive letter")
 	}
 
 	if system.IsAbs(p, platform.OS) {
-		return system.NormalizePath("/", p, platform.OS, true)
+		return system.NormalizePath("/", p, platform.OS, keepSlash)
 	}
 
 	// 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)
+	return system.NormalizePath(dir, p, platform.OS, keepSlash)
 }
 
 func addEnv(env []string, k, v string) []string {

frontend/dockerfile/dockerfile_test.go

@@ -155,6 +155,7 @@ var allTests = integration.TestFuncs(
 	testNamedMultiplatformInputContext,
 	testNamedFilteredContext,
 	testEmptyDestDir,
+	testPreserveDestDirSlash,
 	testCopyLinkDotDestDir,
 	testCopyLinkEmptyDestDir,
 	testCopyChownCreateDest,
@@ -551,6 +552,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)

solver/llbsolver/file/backend.go

@@ -380,19 +380,6 @@ func (fb *Backend) readUserWrapper(owner *pb.ChownOpt, user, group fileoptypes.M
 }
 
 func cleanPath(s string) (string, error) {
-	s, err := system.CheckSystemDriveAndRemoveDriveLetter(s, runtime.GOOS)
-	if err != nil {
-		return "", errors.Wrap(err, "removing drive letter")
-	}
-	s = filepath.FromSlash(s)
-	s2 := filepath.Join("/", s)
-	if strings.HasSuffix(s, string(filepath.Separator)+".") {
-		if s2 != string(filepath.Separator) {
-			s2 += string(filepath.Separator)
-		}
-		s2 += "."
-	} else if strings.HasSuffix(s, string(filepath.Separator)) && s2 != string(filepath.Separator) {
-		s2 += string(filepath.Separator)
-	}
-	return s2, nil
+	// calls the new system.cleanPath
+	return system.CheckSystemDriveAndRemoveDriveLetter(s, runtime.GOOS, true)
 }

util/system/path.go

@@ -39,14 +39,13 @@ func NormalizePath(parent, newPath, inputOS string, keepSlash bool) (string, err
 
 	newPath = ToSlash(newPath, inputOS)
 	parent = ToSlash(parent, inputOS)
-	origPath := newPath
 
 	if parent == "" {
 		parent = "/"
 	}
 
 	var err error
-	parent, err = CheckSystemDriveAndRemoveDriveLetter(parent, inputOS)
+	parent, err = CheckSystemDriveAndRemoveDriveLetter(parent, inputOS, keepSlash)
 	if err != nil {
 		return "", errors.Wrap(err, "removing drive letter")
 	}
@@ -61,7 +60,7 @@ func NormalizePath(parent, newPath, inputOS string, keepSlash bool) (string, err
 		newPath = parent
 	}
 
-	newPath, err = CheckSystemDriveAndRemoveDriveLetter(newPath, inputOS)
+	newPath, err = CheckSystemDriveAndRemoveDriveLetter(newPath, inputOS, keepSlash)
 	if err != nil {
 		return "", errors.Wrap(err, "removing drive letter")
 	}
@@ -71,17 +70,6 @@ func NormalizePath(parent, newPath, inputOS string, keepSlash bool) (string, err
 		newPath = path.Join(parent, newPath)
 	}
 
-	if keepSlash {
-		if strings.HasSuffix(origPath, "/") && !strings.HasSuffix(newPath, "/") {
-			newPath += "/"
-		} else if strings.HasSuffix(origPath, "/.") {
-			if newPath != "/" {
-				newPath += "/"
-			}
-			newPath += "."
-		}
-	}
-
 	return ToSlash(newPath, inputOS), nil
 }
 
@@ -137,7 +125,7 @@ func IsAbs(pth, inputOS string) bool {
 	if inputOS == "" {
 		inputOS = "linux"
 	}
-	cleanedPath, err := CheckSystemDriveAndRemoveDriveLetter(pth, inputOS)
+	cleanedPath, err := CheckSystemDriveAndRemoveDriveLetter(pth, inputOS, false)
 	if err != nil {
 		return false
 	}
@@ -174,7 +162,7 @@ func IsAbs(pth, inputOS string) bool {
 // There is no sane way to support this without adding a lot of complexity
 // which I am not sure is worth it.
 // \\.\C$\a     --> 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 +181,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 +209,31 @@ 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.
+// Returns path with platform specific separators.
+// See https://github.com/moby/buildkit/issues/5249
+func cleanPath(origPath, inputOS string, keepSlash bool) string {
+	if !keepSlash {
+		return filepath.Clean(origPath)
+	}
+
+	// so as to handle cases like \\a\\b\\..\\c\\
+	// on Linux, when inputOS is Windows
+	origPath = ToSlash(origPath, inputOS)
+
+	cleanedPath := filepath.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
 }

util/system/path_test.go

@@ -87,82 +87,119 @@ 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:")
 	}
 
 	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" {
 		t.Fatalf("Single character should be unchanged")
 	}
 
 	// 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" {
 		t.Fatalf("2 characters without colon should be unchanged")
 	}
 
 	// 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` {
 		t.Fatalf("abs path without drive letter should be unchanged")
 	}
 
 	// 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` {
 		t.Fatalf("abs path without drive letter linux failed %s", path)
 	}
 
 	// 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 != `/` {
 		t.Fatalf(`An absolute path should have been shortened to \ %s`, path)
 	}
 
 	// 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 != `/` {
 		t.Fatalf(`A linux style absolute path should have been shortened to \ %s`, path)
 	}
 
 	// 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:"` {
 		t.Fatalf(path, err)
 	}
 
 	// 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:"` {
 		t.Fatalf(path, err)
 	}
 
 	// 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]")
+	}
 }
 
 // TestNormalizeWorkdir tests NormalizeWorkdir