Skip to content

Commit bfde58e

Browse files
ambarveambarve
committed
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 <[email protected]>
1 parent ed1a762 commit bfde58e

2 files changed

Lines changed: 90 additions & 46 deletions

File tree

‎pkg/cri/opts/spec_windows.go‎

Lines changed: 59 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,61 @@ func cleanMount(p string) string {
6161
return filepath.Clean(p)
6262
}
6363

64+
func parseMount(osi osinterface.OS, mount *runtime.Mount) (*runtimespec.Mount, error) {
65+
var (
66+
dst = mount.GetContainerPath()
67+
src = mount.GetHostPath()
68+
)
69+
// In the case of a named pipe mount on Windows, don't stat the file
70+
// or do other operations that open it, as that could interfere with
71+
// the listening process. filepath.Clean also breaks named pipe
72+
// paths, so don't use it.
73+
if !namedPipePath(src) {
74+
if _, err := osi.Stat(src); err != nil {
75+
// Create the host path if it doesn't exist. This will align
76+
// the behavior with the Linux implementation, but it doesn't
77+
// align with Docker's behavior on Windows.
78+
if !os.IsNotExist(err) {
79+
return nil, fmt.Errorf("failed to stat %q: %w", src, err)
80+
}
81+
if err := osi.MkdirAll(src, 0755); err != nil {
82+
return nil, fmt.Errorf("failed to mkdir %q: %w", src, err)
83+
}
84+
}
85+
var err error
86+
src, err = osi.ResolveSymbolicLink(src)
87+
if err != nil {
88+
return nil, fmt.Errorf("failed to resolve symlink %q: %w", src, err)
89+
}
90+
// hcsshim requires clean path, especially '/' -> '\'. Additionally,
91+
// for the destination, absolute paths should have the C: prefix.
92+
src = filepath.Clean(src)
93+
94+
// filepath.Clean adds a '.' at the end if the path is a
95+
// drive (like Z:, E: etc.). Keeping this '.' in the path
96+
// causes incorrect parameter error when starting the
97+
// container on windows. Remove it here.
98+
if !(len(dst) == 2 && dst[1] == ':') {
99+
dst = filepath.Clean(dst)
100+
if dst[0] == '\\' {
101+
dst = "C:" + dst
102+
}
103+
} else if dst[0] == 'c' || dst[0] == 'C' {
104+
return nil, fmt.Errorf("destination path can not be C drive")
105+
}
106+
}
107+
108+
var options []string
109+
// NOTE(random-liu): we don't change all mounts to `ro` when root filesystem
110+
// is readonly. This is different from docker's behavior, but make more sense.
111+
if mount.GetReadonly() {
112+
options = append(options, "ro")
113+
} else {
114+
options = append(options, "rw")
115+
}
116+
return &runtimespec.Mount{Source: src, Destination: dst, Options: options}, nil
117+
}
118+
64119
// WithWindowsMounts sorts and adds runtime and CRI mounts to the spec for
65120
// windows container.
66121
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
110165
}
111166

112167
for _, mount := range mounts {
113-
var (
114-
dst = mount.GetContainerPath()
115-
src = mount.GetHostPath()
116-
)
117-
// In the case of a named pipe mount on Windows, don't stat the file
118-
// or do other operations that open it, as that could interfere with
119-
// the listening process. filepath.Clean also breaks named pipe
120-
// paths, so don't use it.
121-
if !namedPipePath(src) {
122-
if _, err := osi.Stat(src); err != nil {
123-
// Create the host path if it doesn't exist. This will align
124-
// the behavior with the Linux implementation, but it doesn't
125-
// align with Docker's behavior on Windows.
126-
if !os.IsNotExist(err) {
127-
return fmt.Errorf("failed to stat %q: %w", src, err)
128-
}
129-
if err := osi.MkdirAll(src, 0755); err != nil {
130-
return fmt.Errorf("failed to mkdir %q: %w", src, err)
131-
}
132-
}
133-
var err error
134-
src, err = osi.ResolveSymbolicLink(src)
135-
if err != nil {
136-
return fmt.Errorf("failed to resolve symlink %q: %w", src, err)
137-
}
138-
// hcsshim requires clean path, especially '/' -> '\'. Additionally,
139-
// for the destination, absolute paths should have the C: prefix.
140-
src = filepath.Clean(src)
141-
dst = filepath.Clean(dst)
142-
if dst[0] == '\\' {
143-
dst = "C:" + dst
144-
}
145-
}
146-
147-
var options []string
148-
// NOTE(random-liu): we don't change all mounts to `ro` when root filesystem
149-
// is readonly. This is different from docker's behavior, but make more sense.
150-
if mount.GetReadonly() {
151-
options = append(options, "ro")
152-
} else {
153-
options = append(options, "rw")
168+
parsedMount, err := parseMount(osi, mount)
169+
if err != nil {
170+
return err
154171
}
155-
s.Mounts = append(s.Mounts, runtimespec.Mount{
156-
Source: src,
157-
Destination: dst,
158-
Options: options,
159-
})
172+
s.Mounts = append(s.Mounts, *parsedMount)
160173
}
161174
return nil
162175
}

‎pkg/cri/opts/spec_windows_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,14 @@ package opts
1818

1919
import (
2020
"context"
21+
"fmt"
22+
"strings"
2123
"testing"
2224

2325
"github.com/containerd/containerd/containers"
2426
"github.com/containerd/containerd/namespaces"
2527
"github.com/containerd/containerd/oci"
28+
osinterface "github.com/containerd/containerd/pkg/os"
2629
"github.com/opencontainers/runtime-spec/specs-go"
2730
"github.com/stretchr/testify/assert"
2831
"github.com/stretchr/testify/require"
@@ -215,3 +218,31 @@ func TestWithDevices(t *testing.T) {
215218
})
216219
}
217220
}
221+
222+
func TestDriveMounts(t *testing.T) {
223+
tests := []struct {
224+
mnt *runtime.Mount
225+
expectedContainerPath string
226+
expectedError error
227+
}{
228+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\foo`}, `D:\foo`, nil},
229+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:\`}, `D:\`, nil},
230+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `D:`}, `D:`, nil},
231+
{&runtime.Mount{HostPath: `\\.\pipe\a_fake_pipe_name_that_shouldnt_exist`, ContainerPath: `\\.\pipe\foo`}, `\\.\pipe\foo`, nil},
232+
// If `C:\` is passed as container path it should continue and forward that to HCS and fail
233+
// to align with docker's behavior.
234+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:\`}, `C:\`, nil},
235+
236+
// If `C:` is passed we can detect and fail immediately.
237+
{&runtime.Mount{HostPath: `C:\`, ContainerPath: `C:`}, ``, fmt.Errorf("destination path can not be C drive")},
238+
}
239+
var realOS osinterface.RealOS
240+
for _, test := range tests {
241+
parsedMount, err := parseMount(realOS, test.mnt)
242+
if err != nil && !strings.EqualFold(err.Error(), test.expectedError.Error()) {
243+
t.Fatalf("expected err: %s, got %s instead", test.expectedError, err)
244+
} else if err == nil && test.expectedContainerPath != parsedMount.Destination {
245+
t.Fatalf("expected container path: %s, got %s instead", test.expectedContainerPath, parsedMount.Destination)
246+
}
247+
}
248+
}

0 commit comments

Comments
 (0)