Skip to content

Commit eac9331

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #5925 from nalind/mount-flags-parsing
Add more checks to the --mount flag parsing logic
2 parents 833420e + 9b9c161 commit eac9331

File tree

2 files changed

+153
-53
lines changed

2 files changed

+153
-53
lines changed

‎internal/volumes/volumes.go

+85-53
Original file line numberDiff line numberDiff line change
@@ -47,11 +47,12 @@ const (
4747
)
4848

4949
var (
50-
errBadMntOption = errors.New("invalid mount option")
51-
errBadOptionArg = errors.New("must provide an argument for option")
52-
errBadVolDest = errors.New("must set volume destination")
53-
errBadVolSrc = errors.New("must set volume source")
54-
errDuplicateDest = errors.New("duplicate mount destination")
50+
errBadMntOption = errors.New("invalid mount option")
51+
errBadOptionArg = errors.New("must provide an argument for option")
52+
errBadOptionNoArg = errors.New("must not provide an argument for option")
53+
errBadVolDest = errors.New("must set volume destination")
54+
errBadVolSrc = errors.New("must set volume source")
55+
errDuplicateDest = errors.New("duplicate mount destination")
5556
)
5657

5758
// CacheParent returns a cache parent for --mount=type=cache
@@ -144,33 +145,43 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st
144145
argName, argValue, hasArgValue := strings.Cut(val, "=")
145146
switch argName {
146147
case "type":
147-
// This is already processed
148+
// This is already processed, and should be "bind"
148149
continue
149150
case "bind-nonrecursive":
151+
if hasArgValue {
152+
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
153+
}
150154
newMount.Options = append(newMount.Options, "bind")
151155
bindNonRecursive = true
152156
case "nosuid", "nodev", "noexec":
153-
// TODO: detect duplication of these options.
154-
// (Is this necessary?)
157+
if hasArgValue {
158+
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
159+
}
155160
newMount.Options = append(newMount.Options, argName)
156161
case "rw", "readwrite":
162+
if hasArgValue {
163+
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
164+
}
157165
newMount.Options = append(newMount.Options, "rw")
158166
mountReadability = "rw"
159167
case "ro", "readonly":
168+
if hasArgValue {
169+
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
170+
}
160171
newMount.Options = append(newMount.Options, "ro")
161172
mountReadability = "ro"
162173
case "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z", "U", "no-dereference":
163174
if hasArgValue {
164-
return newMount, "", "", "", fmt.Errorf("%v: %w", val, errBadOptionArg)
175+
return newMount, "", "", "", fmt.Errorf("%v: %w", val, errBadOptionNoArg)
165176
}
166177
newMount.Options = append(newMount.Options, argName)
167178
case "from":
168-
if !hasArgValue {
179+
if !hasArgValue || argValue == "" {
169180
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg)
170181
}
171182
fromImage = argValue
172183
case "bind-propagation":
173-
if !hasArgValue {
184+
if !hasArgValue || argValue == "" {
174185
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg)
175186
}
176187
switch argValue {
@@ -181,12 +192,12 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st
181192
}
182193
newMount.Options = append(newMount.Options, argValue)
183194
case "src", "source":
184-
if !hasArgValue {
195+
if !hasArgValue || argValue == "" {
185196
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg)
186197
}
187198
newMount.Source = argValue
188199
case "target", "dst", "destination":
189-
if !hasArgValue {
200+
if !hasArgValue || argValue == "" {
190201
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg)
191202
}
192203
targetPath := argValue
@@ -199,6 +210,9 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st
199210
}
200211
newMount.Destination = targetPath
201212
case "relabel":
213+
if !hasArgValue || argValue == "" {
214+
return newMount, "", "", "", fmt.Errorf("%v: %w", argName, errBadOptionArg)
215+
}
202216
if setRelabel != "" {
203217
return newMount, "", "", "", fmt.Errorf("cannot pass 'relabel' option more than once: %w", errBadOptionArg)
204218
}
@@ -248,6 +262,7 @@ func GetBindMount(sys *types.SystemContext, args []string, contextDir string, st
248262
return newMount, "", "", "", err
249263
}
250264
mountedImage = image.ID()
265+
// unmount the image if we don't end up returning successfully
251266
defer func() {
252267
if !succeeded {
253268
if _, err := store.UnmountImage(mountedImage, false); err != nil {
@@ -335,12 +350,10 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag
335350
var err error
336351
var mode uint64
337352
var buildahLockFilesDir string
338-
var (
339-
setDest bool
340-
setShared bool
341-
setReadOnly bool
342-
foundSElinuxLabel bool
343-
)
353+
var setShared bool
354+
setDest := ""
355+
setRelabel := ""
356+
setReadOnly := ""
344357
fromStage := ""
345358
newMount := specs.Mount{
346359
Type: define.TypeBind,
@@ -360,28 +373,44 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag
360373
argName, argValue, hasArgValue := strings.Cut(val, "=")
361374
switch argName {
362375
case "type":
363-
// This is already processed
376+
// This is already processed, and should be "cache"
364377
continue
365-
case "nosuid", "nodev", "noexec":
366-
// TODO: detect duplication of these options.
367-
// (Is this necessary?)
378+
case "nosuid", "nodev", "noexec", "U":
379+
if hasArgValue {
380+
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
381+
}
368382
newMount.Options = append(newMount.Options, argName)
369383
case "rw", "readwrite":
384+
if hasArgValue {
385+
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
386+
}
370387
newMount.Options = append(newMount.Options, "rw")
388+
setReadOnly = "rw"
371389
case "readonly", "ro":
372-
// Alias for "ro"
390+
if hasArgValue {
391+
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
392+
}
373393
newMount.Options = append(newMount.Options, "ro")
374-
setReadOnly = true
394+
setReadOnly = "ro"
375395
case "Z", "z":
396+
if hasArgValue {
397+
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
398+
}
376399
newMount.Options = append(newMount.Options, argName)
377-
foundSElinuxLabel = true
378-
case "shared", "rshared", "private", "rprivate", "slave", "rslave", "U":
400+
setRelabel = argName
401+
case "shared", "rshared", "private", "rprivate", "slave", "rslave":
402+
if hasArgValue {
403+
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
404+
}
379405
newMount.Options = append(newMount.Options, argName)
380406
setShared = true
381407
case "sharing":
408+
if !hasArgValue || argValue == "" {
409+
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
410+
}
382411
sharing = argValue
383412
case "bind-propagation":
384-
if !hasArgValue {
413+
if !hasArgValue || argValue == "" {
385414
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
386415
}
387416
switch argValue {
@@ -392,17 +421,17 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag
392421
}
393422
newMount.Options = append(newMount.Options, argValue)
394423
case "id":
395-
if !hasArgValue {
424+
if !hasArgValue || argValue == "" {
396425
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
397426
}
398427
id = argValue
399428
case "from":
400-
if !hasArgValue {
429+
if !hasArgValue || argValue == "" {
401430
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
402431
}
403432
fromStage = argValue
404433
case "target", "dst", "destination":
405-
if !hasArgValue {
434+
if !hasArgValue || argValue == "" {
406435
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
407436
}
408437
targetPath := argValue
@@ -413,30 +442,30 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag
413442
return newMount, "", nil, err
414443
}
415444
newMount.Destination = targetPath
416-
setDest = true
445+
setDest = targetPath
417446
case "src", "source":
418-
if !hasArgValue {
447+
if !hasArgValue || argValue == "" {
419448
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
420449
}
421450
newMount.Source = argValue
422451
case "mode":
423-
if !hasArgValue {
452+
if !hasArgValue || argValue == "" {
424453
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
425454
}
426455
mode, err = strconv.ParseUint(argValue, 8, 32)
427456
if err != nil {
428457
return newMount, "", nil, fmt.Errorf("unable to parse cache mode: %w", err)
429458
}
430459
case "uid":
431-
if !hasArgValue {
460+
if !hasArgValue || argValue == "" {
432461
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
433462
}
434463
uid, err = strconv.Atoi(argValue)
435464
if err != nil {
436465
return newMount, "", nil, fmt.Errorf("unable to parse cache uid: %w", err)
437466
}
438467
case "gid":
439-
if !hasArgValue {
468+
if !hasArgValue || argValue == "" {
440469
return newMount, "", nil, fmt.Errorf("%v: %w", argName, errBadOptionArg)
441470
}
442471
gid, err = strconv.Atoi(argValue)
@@ -450,11 +479,11 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag
450479

451480
// If selinux is enabled and no selinux option was configured
452481
// default to `z` i.e shared content label.
453-
if !foundSElinuxLabel && (selinux.EnforceMode() != selinux.Disabled) && fromStage == "" {
482+
if setRelabel == "" && (selinux.EnforceMode() != selinux.Disabled) && fromStage == "" {
454483
newMount.Options = append(newMount.Options, "z")
455484
}
456485

457-
if !setDest {
486+
if setDest == "" {
458487
return newMount, "", nil, errBadVolDest
459488
}
460489

@@ -478,7 +507,7 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag
478507
}
479508
thisCacheRoot = mountPoint
480509
} else {
481-
// we need to create the cache directory on the host if no image is being used
510+
// we need to create the cache directory on the host if no stage is being used
482511

483512
// since type is cache and a cache can be reused by consecutive builds
484513
// create a common cache directory, which persists on hosts within temp lifecycle
@@ -561,8 +590,8 @@ func GetCacheMount(args []string, additionalMountPoints map[string]internal.Stag
561590
newMount.Options = append(newMount.Options, "shared")
562591
}
563592

564-
// buildkit parity: cache must writable unless `ro` or `readonly` is configured explicitly
565-
if !setReadOnly {
593+
// buildkit parity: cache must be writable unless `ro` or `readonly` is configured explicitly
594+
if setReadOnly == "" {
566595
newMount.Options = append(newMount.Options, "rw")
567596
}
568597

@@ -727,9 +756,6 @@ func getMounts(ctx *types.SystemContext, store storage.Store, mountLabel string,
727756

728757
errInvalidSyntax := errors.New("incorrect mount format: should be --mount type=<bind|tmpfs>,[src=<host-dir>,]target=<ctr-dir>[,options]")
729758

730-
// TODO(vrothberg): the manual parsing can be replaced with a regular expression
731-
// to allow a more robust parsing of the mount format and to give
732-
// precise errors regarding supported format versus supported options.
733759
for _, mount := range mounts {
734760
tokens := strings.Split(mount, ",")
735761
if len(tokens) < 2 {
@@ -809,30 +835,36 @@ func GetTmpfsMount(args []string, workDir string) (specs.Mount, error) {
809835
argName, argValue, hasArgValue := strings.Cut(val, "=")
810836
switch argName {
811837
case "type":
812-
// This is already processed
838+
// This is already processed, and should be "tmpfs"
813839
continue
814-
case "ro", "nosuid", "nodev", "noexec":
840+
case "nosuid", "nodev", "noexec":
841+
if hasArgValue {
842+
return newMount, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
843+
}
815844
newMount.Options = append(newMount.Options, argName)
816-
case "readonly":
817-
// Alias for "ro"
845+
case "ro", "readonly":
846+
if hasArgValue {
847+
return newMount, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
848+
}
818849
newMount.Options = append(newMount.Options, "ro")
819850
case "tmpcopyup":
851+
if hasArgValue {
852+
return newMount, fmt.Errorf("%v: %w", argName, errBadOptionNoArg)
853+
}
820854
// the path that is shadowed by the tmpfs mount is recursively copied up to the tmpfs itself.
821855
newMount.Options = append(newMount.Options, argName)
822856
case "tmpfs-mode":
823-
if !hasArgValue {
857+
if !hasArgValue || argValue == "" {
824858
return newMount, fmt.Errorf("%v: %w", argName, errBadOptionArg)
825859
}
826860
newMount.Options = append(newMount.Options, fmt.Sprintf("mode=%s", argValue))
827861
case "tmpfs-size":
828-
if !hasArgValue {
862+
if !hasArgValue || argValue == "" {
829863
return newMount, fmt.Errorf("%v: %w", argName, errBadOptionArg)
830864
}
831865
newMount.Options = append(newMount.Options, fmt.Sprintf("size=%s", argValue))
832-
case "src", "source":
833-
return newMount, errors.New("source is not supported with tmpfs mounts")
834866
case "target", "dst", "destination":
835-
if !hasArgValue {
867+
if !hasArgValue || argValue == "" {
836868
return newMount, fmt.Errorf("%v: %w", argName, errBadOptionArg)
837869
}
838870
targetPath := argValue

‎internal/volumes/volumes_test.go

+68
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package volumes
2+
3+
import (
4+
"testing"
5+
6+
"github.com/containers/image/v5/types"
7+
"github.com/containers/storage"
8+
storageTypes "github.com/containers/storage/types"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
func TestGetMount(t *testing.T) {
14+
tempDir := t.TempDir()
15+
rootDir := t.TempDir()
16+
runDir := t.TempDir()
17+
18+
store, err := storage.GetStore(storageTypes.StoreOptions{
19+
GraphDriverName: "vfs",
20+
GraphRoot: rootDir,
21+
RunRoot: runDir,
22+
})
23+
require.NoError(t, err)
24+
t.Cleanup(func() {
25+
if _, err := store.Shutdown(true); err != nil {
26+
t.Logf("shutting down temporary store: %v", err)
27+
}
28+
})
29+
30+
t.Run("GetBindMount", func(t *testing.T) {
31+
for _, argNeeder := range []string{"from", "bind-propagation", "src", "source", "target", "dst", "destination", "relabel"} {
32+
_, _, _, _, err := GetBindMount(&types.SystemContext{}, []string{argNeeder}, tempDir, store, "", nil, tempDir, tempDir)
33+
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
34+
_, _, _, _, err = GetBindMount(&types.SystemContext{}, []string{argNeeder + "="}, tempDir, store, "", nil, tempDir, tempDir)
35+
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
36+
}
37+
for _, argHater := range []string{"bind-nonrecursive", "nodev", "noexec", "nosuid", "ro", "readonly", "rw", "readwrite", "shared", "rshared", "private", "rprivate", "slave", "rslave", "Z", "z", "U", "no-dereference"} {
38+
_, _, _, _, err := GetBindMount(&types.SystemContext{}, []string{argHater + "=nonce"}, tempDir, store, "", nil, tempDir, tempDir)
39+
assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater)
40+
}
41+
})
42+
43+
t.Run("GetCacheMount", func(t *testing.T) {
44+
for _, argNeeder := range []string{"sharing", "id", "from", "bind-propagation", "src", "source", "target", "dst", "destination", "mode", "uid", "gid"} {
45+
_, _, _, err := GetCacheMount([]string{argNeeder}, nil, tempDir, tempDir)
46+
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
47+
_, _, _, err = GetCacheMount([]string{argNeeder + "="}, nil, tempDir, tempDir)
48+
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
49+
}
50+
for _, argHater := range []string{"nodev", "noexec", "nosuid", "U", "rw", "readwrite", "ro", "readonly", "shared", "Z", "z", "rshared", "private", "rprivate", "slave", "rslave"} {
51+
_, _, _, err := GetCacheMount([]string{argHater + "=nonce"}, nil, tempDir, tempDir)
52+
assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater)
53+
}
54+
})
55+
56+
t.Run("GetTmpfsMount", func(t *testing.T) {
57+
for _, argNeeder := range []string{"tmpfs-mode", "tmpfs-size", "target", "dst", "destination"} {
58+
_, err := GetTmpfsMount([]string{argNeeder}, tempDir)
59+
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
60+
_, err = GetTmpfsMount([]string{argNeeder + "="}, tempDir)
61+
assert.ErrorIsf(t, err, errBadOptionArg, "option %q was supposed to have an arg, but wasn't flagged when it didn't have one", argNeeder)
62+
}
63+
for _, argHater := range []string{"nodev", "noexec", "nosuid", "ro", "readonly", "tmpcopyup"} {
64+
_, err := GetTmpfsMount([]string{argHater + "=nonce"}, tempDir)
65+
assert.ErrorIsf(t, err, errBadOptionNoArg, "option %q is not supposed to have an arg, but wasn't flagged when it tried to supply one", argHater)
66+
}
67+
})
68+
}

0 commit comments

Comments
 (0)