Bug fix for mount path handling

Currently when handling 'container_path' elements in container mounts we simply call
filepath.Clean on those paths. However, filepath.Clean adds an extra '.' if the path is a
simple drive letter ('E:' or 'Z:' etc.). These type of paths cause failures (with incorrect
parameter error) when creating containers via hcsshim. This commit checks for such paths
and doesn't call filepath.Clean on them.
It also adds a new check to error out if the destination path is a C drive and moves the
dst path checks out of the named pipe condition.

Signed-off-by: Amit Barve <ambarve@microsoft.com>
(cherry picked from commit bfde58e)
Signed-off-by: Amit Barve <ambarve@microsoft.com>

pkg/cri/opts/spec_windows.go

@@ -61,6 +61,61 @@ func cleanMount(p string) string {
 	return filepath.Clean(p)
 }
 
+func parseMount(osi osinterface.OS, mount *runtime.Mount) (*runtimespec.Mount, error) {
+	var (
+		dst = mount.GetContainerPath()
+		src = mount.GetHostPath()
+	)
+	// In the case of a named pipe mount on Windows, don't stat the file
+	// or do other operations that open it, as that could interfere with
+	// the listening process. filepath.Clean also breaks named pipe
+	// paths, so don't use it.
+	if !namedPipePath(src) {
+		if _, err := osi.Stat(src); err != nil {
+			// Create the host path if it doesn't exist. This will align
+			// the behavior with the Linux implementation, but it doesn't
+			// align with Docker's behavior on Windows.
+			if !os.IsNotExist(err) {
+				return nil, fmt.Errorf("failed to stat %q: %w", src, err)
+			}
+			if err := osi.MkdirAll(src, 0755); err != nil {
+				return nil, fmt.Errorf("failed to mkdir %q: %w", src, err)
+			}
+		}
+		var err error
+		src, err = osi.ResolveSymbolicLink(src)
+		if err != nil {
+			return nil, fmt.Errorf("failed to resolve symlink %q: %w", src, err)
+		}
+		// hcsshim requires clean path, especially '/' -> '\'. Additionally,
+		// for the destination, absolute paths should have the C: prefix.
+		src = filepath.Clean(src)
+
+		// filepath.Clean adds a '.' at the end if the path is a
+		// drive (like Z:, E: etc.). Keeping this '.' in the path
+		// causes incorrect parameter error when starting the
+		// container on windows.  Remove it here.
+		if !(len(dst) == 2 && dst[1] == ':') {
+			dst = filepath.Clean(dst)
+			if dst[0] == '\\' {
+				dst = "C:" + dst
+			}
+		} else if dst[0] == 'c' || dst[0] == 'C' {
+			return nil, fmt.Errorf("destination path can not be C drive")
+		}
+	}
+
+	var options []string
+	// NOTE(random-liu): we don't change all mounts to `ro` when root filesystem
+	// is readonly. This is different from docker's behavior, but make more sense.
+	if mount.GetReadonly() {
+		options = append(options, "ro")
+	} else {
+		options = append(options, "rw")
+	}
+	return &runtimespec.Mount{Source: src, Destination: dst, Options: options}, nil
+}
+
 // WithWindowsMounts sorts and adds runtime and CRI mounts to the spec for
 // windows container.
 func WithWindowsMounts(osi osinterface.OS, config *runtime.ContainerConfig, extra []*runtime.Mount) oci.SpecOpts {
@@ -110,53 +165,11 @@ func WithWindowsMounts(osi osinterface.OS, config *runtime.ContainerConfig, extr
 		}
 
 		for _, mount := range mounts {
-			var (
-				dst = mount.GetContainerPath()
-				src = mount.GetHostPath()
-			)
-			// In the case of a named pipe mount on Windows, don't stat the file
-			// or do other operations that open it, as that could interfere with
-			// the listening process. filepath.Clean also breaks named pipe
-			// paths, so don't use it.
-			if !namedPipePath(src) {
-				if _, err := osi.Stat(src); err != nil {
-					// Create the host path if it doesn't exist. This will align
-					// the behavior with the Linux implementation, but it doesn't
-					// align with Docker's behavior on Windows.
-					if !os.IsNotExist(err) {
-						return fmt.Errorf("failed to stat %q: %w", src, err)
-					}
-					if err := osi.MkdirAll(src, 0755); err != nil {
-						return fmt.Errorf("failed to mkdir %q: %w", src, err)
-					}
-				}
-				var err error
-				src, err = osi.ResolveSymbolicLink(src)
-				if err != nil {
-					return fmt.Errorf("failed to resolve symlink %q: %w", src, err)
-				}
-				// hcsshim requires clean path, especially '/' -> '\'. Additionally,
-				// for the destination, absolute paths should have the C: prefix.
-				src = filepath.Clean(src)
-				dst = filepath.Clean(dst)
-				if dst[0] == '\\' {
-					dst = "C:" + dst
-				}
-			}
-
-			var options []string
-			// NOTE(random-liu): we don't change all mounts to `ro` when root filesystem
-			// is readonly. This is different from docker's behavior, but make more sense.
-			if mount.GetReadonly() {
-				options = append(options, "ro")
-			} else {
-				options = append(options, "rw")
+			parsedMount, err := parseMount(osi, mount)
+			if err != nil {
+				return err
 			}
-			s.Mounts = append(s.Mounts, runtimespec.Mount{
-				Source:      src,
-				Destination: dst,
-				Options:     options,
-			})
+			s.Mounts = append(s.Mounts, *parsedMount)
 		}
 		return nil
 	}

pkg/cri/opts/spec_windows_test.go

@@ -0,0 +1,54 @@
+/*
+   Copyright The containerd Authors.
+
+   Licensed under the Apache License, Version 2.0 (the "License");
+   you may not use this file except in compliance with the License.
+   You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+*/
+
+package opts
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	osinterface "github.com/containerd/containerd/pkg/os"
+	runtime "k8s.io/cri-api/pkg/apis/runtime/v1"
+)
+
+func TestDriveMounts(t *testing.T) {
+	tests := []struct {
+		mnt                   *runtime.Mount
+		expectedContainerPath string
+		expectedError         error
+	}{
+		{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\foo`}, `D:\foo`, nil},
+		{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\`}, `D:\`, nil},
+		{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:`}, `D:`, nil},
+		{&runtime.Mount{HostPath: `\\.\pipe\a_fake_pipe_name_that_shouldnt_exist`, ContainerPath: `\\.\pipe\foo`}, `\\.\pipe\foo`, nil},
+		// If `C:\` is passed as container path it should continue and forward that to HCS and fail
+		// to align with docker's behavior.
+		{&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:\`}, `C:\`, nil},
+
+		// If `C:` is passed we can detect and fail immediately.
+		{&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:`}, ``, fmt.Errorf("destination path can not be C drive")},
+	}
+	var realOS osinterface.RealOS
+	for _, test := range tests {
+		parsedMount, err := parseMount(realOS, test.mnt)
+		if err != nil && !strings.EqualFold(err.Error(), test.expectedError.Error()) {
+			t.Fatalf("expected err: %s, got %s instead", test.expectedError, err)
+		} else if err == nil && test.expectedContainerPath != parsedMount.Destination {
+			t.Fatalf("expected container path: %s, got %s instead", test.expectedContainerPath, parsedMount.Destination)
+		}
+	}
+}