Skip to content

Commit cf64923

Browse files
wkingwking
committed
filepath: Add a stand-alone package for explicit-OS path logic
Go's path/filepath has lots of useful path operations, but there is a compile-time decision to select only the logic that applies to your $GOOS. That makes it hard to validate a Windows config from a Linux host (or vice versa) because Go's builtin tools won't tell you whether a path is absolute on the target platform (just whether the path is absolute on *your* platform). This commit adds a new package to do the same sorts of things but with an explicit OS argument. In some cases, there's also an explicit workding directory argument. For example, Go's Abs has [1]: If the path is not absolute it will be joined with the current working directory to turn it into an absolute path. but that doesn't make sense for a cross-platform Abs call because the real current working directory will be for the wrong platform. Instead, cross-platform calls to Abs and similar should fake a working directory as if they were being called from the other platform. The Windows implementation is not very complete, with IsAbs definitely missing a lot of stuff; Abs, Clean, and IsAncestor probably missing stuff; and a lack of Windows-path tests. But the current tools are broken for validating Windows configs anyway, so I've left completing Windows support to future work. The regular expression I've used is similar to what we used to use in pathValid, but has been adjusted based on [2]: The absolute path has the following format: LocalDrive:\Path\FileName I've assumed that c:\a is absolute as well, even though it doesn't quite match the above pattern. And I no longer test for colons, question marks, and other reserved characters [3], although it would be easy to add checks for that as well if we wanted. Besides adding the new package, I updated the config validation to use the new package where appropriate. For example checks for absolute hook paths (based on [4]) now appropriately account for the target platform (although Abs has limited Windows support at the moment, as mentioned above). There are still a number of config validation checks that use Go's stock filepath, because they're based around actual filesystem access (e.g. reading config.json off the disk, asserting that root.path exists on the disk, etc.). Some of those will need logic to convert between path platforms (which I'm leaving to future work). For example, if root.path is formed for another platform, then: * If root.path is absolute (on the target platform), there's no way to check whether root.path exists inside the bundle. * If root.path is relative, we should be converting it from the target platform to the host platform before joining it to our bundle path. For example, with a Windows bundle in "/foo" on a Linux host where root.path is "bar\baz", then runtime-tools should be checking for the root directory in /foo/bar/baz, not in /foo/bar\baz. The root.path example is somewhat guarded by the bundle requirement for siblinghood [5], but I'd rather enforce siblinghood independently from existence [6], since the spec has separate requirements for both. I'm not clear on why we were using pathValid for mount sources on Windows. We've been doing that since 4dcd484 (validate: add mounts whether nested check, 2016-10-25, #256, [7]), but I see no absolute-path requirement in the spec [8], which only forbids UNC paths and mapped drives for source (which we don't yet have tooling to detect). Perhaps the idea was just to check for paths which contained reserved characters? In any case, I think source handling on Windows should not involve IsAbs and should be addressed more thoroughly in future work. [1]: https://golang.org/pkg/path/filepath/#Abs [2]: https://technet.microsoft.com/en-us/library/ee692607.aspx [3]: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx#naming_conventions [4]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L375 [5]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/bundle.md#L17-L19 [6]: https://github.com/opencontainers/runtime-spec/blame/v1.0.0/config.md#L43 [7]: https://github.com/opencontainers/runtime-tools/pull/256/files#diff-8351286f57eb81e245c9c99c07f3b34fR413 [8]: https://github.com/opencontainers/runtime-spec/blob/v1.0.0/config.md#mounts Signed-off-by: W. Trevor King <[email protected]>
1 parent 12b47b9 commit cf64923

File tree

11 files changed

+534
-84
lines changed

11 files changed

+534
-84
lines changed

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,6 @@ test: .gofmt .govet .golint .gotest
5353
.golint:
5454
golint -set_exit_status $(PACKAGES)
5555

56-
UTDIRS = ./validate/...
56+
UTDIRS = ./filepath/... ./validate/...
5757
.gotest:
5858
go test $(UTDIRS)

‎filepath/abs.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package filepath
2+
3+
import (
4+
"errors"
5+
"regexp"
6+
"strings"
7+
)
8+
9+
var windowsAbs = regexp.MustCompile(`^[a-zA-Z]:\\.*$`)
10+
11+
// Abs is a version of path/filepath's Abs with an explicit operating
12+
// system and current working directory.
13+
func Abs(os, path, cwd string) (_ string, err error) {
14+
if os == "windows" {
15+
return "", errors.New("Abs() does not support windows yet")
16+
}
17+
if IsAbs(os, path) {
18+
return Clean(os, path), nil
19+
}
20+
return Clean(os, Join(os, cwd, path)), nil
21+
}
22+
23+
// IsAbs is a version of path/filepath's IsAbs with an explicit
24+
// operating system.
25+
func IsAbs(os, path string) bool {
26+
if os == "windows" {
27+
// FIXME: copy hideous logic from Go's
28+
// src/path/filepath/path_windows.go into somewhere where we can
29+
// put 3-clause BSD licensed code.
30+
return windowsAbs.MatchString(path)
31+
}
32+
sep := Separator(os)
33+
34+
// POSIX has [1]:
35+
//
36+
// > If a pathname begins with two successive <slash> characters,
37+
// > the first component following the leading <slash> characters
38+
// > may be interpreted in an implementation-defined manner,
39+
// > although more than two leading <slash> characters shall be
40+
// > treated as a single <slash> character.
41+
//
42+
// And Boost treats // as non-absolute [2], but Linux [3,4], Python
43+
// [5] and Go [6] all treat // as absolute.
44+
//
45+
// [1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13
46+
// [2]: https://github.com/boostorg/filesystem/blob/boost-1.64.0/test/path_test.cpp#L861
47+
// [3]: http://man7.org/linux/man-pages/man7/path_resolution.7.html
48+
// [4]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/filesystems/path-lookup.md?h=v4.12#n41
49+
// [5]: https://github.com/python/cpython/blob/v3.6.1/Lib/posixpath.py#L64-L66
50+
// [6]: https://go.googlesource.com/go/+/go1.8.3/src/path/path.go#199
51+
return strings.HasPrefix(path, string(sep))
52+
}

‎filepath/abs_test.go‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
package filepath
2+
3+
import (
4+
"fmt"
5+
"testing"
6+
)
7+
8+
func TestAbs(t *testing.T) {
9+
for _, test := range []struct {
10+
os string
11+
path string
12+
cwd string
13+
expected string
14+
}{
15+
{
16+
os: "linux",
17+
path: "/",
18+
cwd: "/cwd",
19+
expected: "/",
20+
},
21+
{
22+
os: "linux",
23+
path: "/a",
24+
cwd: "/cwd",
25+
expected: "/a",
26+
},
27+
{
28+
os: "linux",
29+
path: "/a/",
30+
cwd: "/cwd",
31+
expected: "/a",
32+
},
33+
{
34+
os: "linux",
35+
path: "//a",
36+
cwd: "/cwd",
37+
expected: "/a",
38+
},
39+
{
40+
os: "linux",
41+
path: ".",
42+
cwd: "/cwd",
43+
expected: "/cwd",
44+
},
45+
{
46+
os: "linux",
47+
path: "./c",
48+
cwd: "/a/b",
49+
expected: "/a/b/c",
50+
},
51+
{
52+
os: "linux",
53+
path: ".//c",
54+
cwd: "/a/b",
55+
expected: "/a/b/c",
56+
},
57+
{
58+
os: "linux",
59+
path: "../a",
60+
cwd: "/cwd",
61+
expected: "/a",
62+
},
63+
{
64+
os: "linux",
65+
path: "../../b",
66+
cwd: "/cwd",
67+
expected: "/b",
68+
},
69+
} {
70+
t.Run(
71+
fmt.Sprintf("Abs(%q,%q,%q)", test.os, test.path, test.cwd),
72+
func(t *testing.T) {
73+
abs, err := Abs(test.os, test.path, test.cwd)
74+
if err != nil {
75+
t.Error(err)
76+
} else if abs != test.expected {
77+
t.Errorf("unexpected result: %q (expected %q)\n", abs, test.expected)
78+
}
79+
},
80+
)
81+
}
82+
}
83+
84+
func TestIsAbs(t *testing.T) {
85+
for _, test := range []struct {
86+
os string
87+
path string
88+
expected bool
89+
}{
90+
{
91+
os: "linux",
92+
path: "/",
93+
expected: true,
94+
},
95+
{
96+
os: "linux",
97+
path: "/a",
98+
expected: true,
99+
},
100+
{
101+
os: "linux",
102+
path: "//",
103+
expected: true,
104+
},
105+
{
106+
os: "linux",
107+
path: "//a",
108+
expected: true,
109+
},
110+
{
111+
os: "linux",
112+
path: ".",
113+
expected: false,
114+
},
115+
{
116+
os: "linux",
117+
path: "./a",
118+
expected: false,
119+
},
120+
{
121+
os: "linux",
122+
path: ".//a",
123+
expected: false,
124+
},
125+
{
126+
os: "linux",
127+
path: "../a",
128+
expected: false,
129+
},
130+
{
131+
os: "linux",
132+
path: "../../a",
133+
expected: false,
134+
},
135+
{
136+
os: "windows",
137+
path: "c:\\",
138+
expected: true,
139+
},
140+
{
141+
os: "windows",
142+
path: "c:\\a",
143+
expected: true,
144+
},
145+
{
146+
os: "windows",
147+
path: ".",
148+
expected: false,
149+
},
150+
{
151+
os: "windows",
152+
path: ".\\a",
153+
expected: false,
154+
},
155+
{
156+
os: "windows",
157+
path: "..\\a",
158+
expected: false,
159+
},
160+
} {
161+
t.Run(
162+
fmt.Sprintf("IsAbs(%q,%q)", test.os, test.path),
163+
func(t *testing.T) {
164+
abs := IsAbs(test.os, test.path)
165+
if abs != test.expected {
166+
t.Errorf("unexpected result: %t (expected %t)\n", abs, test.expected)
167+
}
168+
},
169+
)
170+
}
171+
}

‎filepath/ancestor.go‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package filepath
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
)
7+
8+
// IsAncestor returns true when pathB is an strict ancestor of pathA,
9+
// and false where the paths are equal or pathB is outside of pathA.
10+
// Paths that are not absolute will be made absolute with Abs.
11+
func IsAncestor(os, pathA, pathB, cwd string) (_ bool, err error) {
12+
if pathA == pathB {
13+
return false, nil
14+
}
15+
16+
pathA, err = Abs(os, pathA, cwd)
17+
if err != nil {
18+
return false, err
19+
}
20+
pathB, err = Abs(os, pathB, cwd)
21+
if err != nil {
22+
return false, err
23+
}
24+
sep := Separator(os)
25+
if !strings.HasSuffix(pathA, string(sep)) {
26+
pathA = fmt.Sprintf("%s%c", pathA, sep)
27+
}
28+
if pathA == pathB {
29+
return false, nil
30+
}
31+
return strings.HasPrefix(pathB, pathA), nil
32+
}

‎filepath/ancestor_test.go‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
package filepath
2+
3+
import (
4+
"fmt"
5+
"testing"
6+
)
7+
8+
func TestIsAncestor(t *testing.T) {
9+
for _, test := range []struct {
10+
os string
11+
pathA string
12+
pathB string
13+
cwd string
14+
expected bool
15+
}{
16+
{
17+
os: "linux",
18+
pathA: "/",
19+
pathB: "/a",
20+
cwd: "/cwd",
21+
expected: true,
22+
},
23+
{
24+
os: "linux",
25+
pathA: "/a",
26+
pathB: "/a",
27+
cwd: "/cwd",
28+
expected: false,
29+
},
30+
{
31+
os: "linux",
32+
pathA: "/a",
33+
pathB: "/",
34+
cwd: "/cwd",
35+
expected: false,
36+
},
37+
{
38+
os: "linux",
39+
pathA: "/a",
40+
pathB: "/ab",
41+
cwd: "/cwd",
42+
expected: false,
43+
},
44+
{
45+
os: "linux",
46+
pathA: "/a/",
47+
pathB: "/a",
48+
cwd: "/cwd",
49+
expected: false,
50+
},
51+
{
52+
os: "linux",
53+
pathA: "//a",
54+
pathB: "/a",
55+
cwd: "/cwd",
56+
expected: false,
57+
},
58+
{
59+
os: "linux",
60+
pathA: "//a",
61+
pathB: "/a/b",
62+
cwd: "/cwd",
63+
expected: true,
64+
},
65+
{
66+
os: "linux",
67+
pathA: "/a",
68+
pathB: "/a/",
69+
cwd: "/cwd",
70+
expected: false,
71+
},
72+
{
73+
os: "linux",
74+
pathA: "/a",
75+
pathB: ".",
76+
cwd: "/cwd",
77+
expected: false,
78+
},
79+
{
80+
os: "linux",
81+
pathA: "/a",
82+
pathB: "b",
83+
cwd: "/a",
84+
expected: true,
85+
},
86+
{
87+
os: "linux",
88+
pathA: "/a",
89+
pathB: "../a",
90+
cwd: "/cwd",
91+
expected: false,
92+
},
93+
{
94+
os: "linux",
95+
pathA: "/a",
96+
pathB: "../a/b",
97+
cwd: "/cwd",
98+
expected: true,
99+
},
100+
} {
101+
t.Run(
102+
fmt.Sprintf("IsAncestor(%q,%q,%q,%q)", test.os, test.pathA, test.pathB, test.cwd),
103+
func(t *testing.T) {
104+
ancestor, err := IsAncestor(test.os, test.pathA, test.pathB, test.cwd)
105+
if err != nil {
106+
t.Error(err)
107+
} else if ancestor != test.expected {
108+
t.Errorf("unexpected result: %t", ancestor)
109+
}
110+
},
111+
)
112+
}
113+
}

0 commit comments

Comments
 (0)