fix: util/path: CheckSystemDriveAndRemoveDriveLetter to preserve /

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")
 	}

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=<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

@@ -5,7 +5,6 @@ import (
 	"log"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"time"
 
@@ -14,7 +13,6 @@ import (
 	"github.com/moby/buildkit/snapshot"
 	"github.com/moby/buildkit/solver/llbsolver/ops/fileoptypes"
 	"github.com/moby/buildkit/solver/pb"
-	"github.com/moby/buildkit/util/system"
 	"github.com/pkg/errors"
 	copy "github.com/tonistiigi/fsutil/copy"
 )
@@ -111,11 +109,7 @@ func rm(d string, action *pb.FileActionRm) (err error) {
 	}()
 
 	if action.AllowWildcard {
-		src, err := cleanPath(action.Path)
-		if err != nil {
-			return errors.Wrap(err, "cleaning path")
-		}
-		m, err := copy.ResolveWildcards(d, src, false)
+		m, err := copy.ResolveWildcards(d, action.Path, false)
 		if err != nil {
 			return errors.WithStack(err)
 		}
@@ -156,14 +150,10 @@ func rmPath(root, src string, allowNotFound bool) error {
 }
 
 func docopy(ctx context.Context, src, dest string, action *pb.FileActionCopy, u *copy.User, idmap *idtools.IdentityMapping) (err error) {
-	srcPath, err := cleanPath(action.Src)
-	if err != nil {
-		return errors.Wrap(err, "cleaning source path")
-	}
-	destPath, err := cleanPath(action.Dest)
-	if err != nil {
-		return errors.Wrap(err, "cleaning destination path")
-	}
+	// NB: the paths do not need cleaning
+	srcPath := action.Src
+	destPath := action.Dest
+
 	if !action.CreateDestPath {
 		p, err := fs.RootPath(dest, filepath.Join("/", action.Dest))
 		if err != nil {
@@ -378,21 +368,3 @@ func (fb *Backend) readUserWrapper(owner *pb.ChownOpt, user, group fileoptypes.M
 	}
 	return u, nil
 }
-
-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
-}

util/system/path.go

@@ -2,7 +2,6 @@ package system
 
 import (
 	"path"
-	"path/filepath"
 	"strings"
 
 	"github.com/pkg/errors"
@@ -46,7 +45,7 @@ func NormalizePath(parent, newPath, inputOS string, keepSlash bool) (string, err
 	}
 
 	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")
 	}
@@ -137,7 +136,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 +173,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 +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
 }

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]")
+	}
 }
 
 // TestNormalizeWorkdirWindows tests NormalizeWorkdir