Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions libcontainer/specconv/spec_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -1141,19 +1141,21 @@ func parseMountOptions(options []string) *configs.Mount {
)
initMaps()
for _, o := range options {
// If the option does not exist in the mountFlags table,
// or the flag is not supported on the platform,
// then it is a data value for a specific fs type.
if f, exists := mountFlags[o]; exists && f.flag != 0 {
// Options absent from the mountFlags table are data for a specific fs type.
// A recognized option is consumed even when its flag is zero: "defaults"
// is a no-op and must not be forwarded as filesystem data (for example to tmpfs).
if f, exists := mountFlags[o]; exists {
// FIXME: The *atime flags are special (they are more of an enum
// with quite hairy semantics) and thus arguably setting some of
// them should clear unrelated flags.
if f.clear {
m.Flags &= ^f.flag
m.ClearedFlags |= f.flag
} else {
m.Flags |= f.flag
m.ClearedFlags &= ^f.flag
if f.flag != 0 {
if f.clear {
m.Flags &= ^f.flag
m.ClearedFlags |= f.flag
} else {
m.Flags |= f.flag
m.ClearedFlags &= ^f.flag
}
Comment on lines -1144 to +1158

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too much of a fix. What's sufficient is:

@@ -1141,10 +1141,9 @@ func parseMountOptions(options []string) *configs.Mount {
      initMaps()
      for _, o := range options {
-             // If the option does not exist in the mountFlags table,
-             // or the flag is not supported on the platform,
-             // then it is a data value for a specific fs type.
-             if f, exists := mountFlags[o]; exists && f.flag != 0 {
+             // If the option does not exist in the mountFlags table,
+             // then it is a data value for a specific fs type.
+             if f, exists := mountFlags[o]; exists {
                      // FIXME: The *atime flags are special (they are more of an enum
                      // with quite hairy semantics) and thus arguably setting some of
                      // them should clear unrelated flags.

and maybe add defaults to the table, for clarity:

  "defaults":      {false, 0}, // no-op, must not be passed as data

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The above also removes the obsoleted "flags is not supported on the platform" as modern x/sys/unix have all the flags defined).

}
} else if f, exists := mountPropagationMapping[o]; exists && f != 0 {
m.PropagationFlags = append(m.PropagationFlags, f)
Expand Down
36 changes: 36 additions & 0 deletions libcontainer/specconv/spec_linux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1026,3 +1026,39 @@ func TestCreateNetDevices(t *testing.T) {
})
}
}

func TestParseMountOptionsDefaultsNotInData(t *testing.T) {
m := parseMountOptions([]string{"defaults", "size=1m", "mode=777"})
if strings.Contains(m.Data, "defaults") {
t.Errorf("defaults must not be passed as mount data, got %q", m.Data)
}
if !strings.Contains(m.Data, "size=1m") || !strings.Contains(m.Data, "mode=777") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather check for a specific string here:

if m.Data != "size=1m,mode=777" {

t.Errorf("expected size and mode in mount data, got %q", m.Data)
}
if m.Flags != 0 || m.ClearedFlags != 0 {
t.Errorf("defaults must not change mount flags, flags=%#x cleared=%#x", m.Flags, m.ClearedFlags)
}

unknown := parseMountOptions([]string{"defaults", "notarealoption", "size=1m"})
if strings.Contains(unknown.Data, "defaults") {
t.Errorf("defaults must not be passed as mount data, got %q", unknown.Data)
}
if !strings.Contains(unknown.Data, "notarealoption") {
t.Errorf("unknown option must be preserved in mount data, got %q", unknown.Data)
}

// Nonzero flags keep set/clear behavior and are not treated as data.
flagged := parseMountOptions([]string{"ro", "nodev", "rw"})
if flagged.Flags&unix.MS_RDONLY != 0 {
t.Errorf("rw should clear MS_RDONLY, flags=%#x cleared=%#x", flagged.Flags, flagged.ClearedFlags)
}
if flagged.ClearedFlags&unix.MS_RDONLY == 0 {
t.Errorf("rw should record MS_RDONLY as cleared, cleared=%#x", flagged.ClearedFlags)
}
if flagged.Flags&unix.MS_NODEV == 0 {
t.Errorf("nodev should set MS_NODEV, flags=%#x", flagged.Flags)
}
if flagged.Data != "" {
t.Errorf("known flags must not be passed as data, got %q", flagged.Data)
}
}