Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chunked: honor the ForceMask setting #1971

Merged
merged 4 commits into from
Jun 20, 2024
Merged

chunked: honor the ForceMask setting #1971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f37aea9
idtools: move format override xattr to new function
giuseppe Jun 13, 2024
e717273
chunked: use FormatContainersOverrideXattr
giuseppe Jun 13, 2024
eec2a60
overlay: pass down ForceMask to chunked
giuseppe Jun 13, 2024
f35833c
chunked: set correctly the xattr for ForceMask
giuseppe Jun 13, 2024
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
11 changes: 9 additions & 2 deletions drivers/overlay/overlay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2162,9 +2162,15 @@ func supportsDataOnlyLayersCached(home, runhome string) (bool, error) {
// ApplyDiffWithDiffer applies the changes in the new layer using the specified function
func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.ApplyDiffWithDifferOpts, differ graphdriver.Differ) (output graphdriver.DriverWithDifferOutput, errRet error) {
var idMappings *idtools.IDMappings
forceMask := options.ForceMask

if options != nil {
Comment on lines +2165 to 2167
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either the first line can crash, or the second one is unnecessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks, opened a PR: #1984

idMappings = options.Mappings
}
if d.options.forceMask != nil {
forceMask = d.options.forceMask
}

if idMappings == nil {
idMappings = &idtools.IDMappings{}
}
Expand All @@ -2182,8 +2188,8 @@ func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.App
return graphdriver.DriverWithDifferOutput{}, err
}
perms := defaultPerms
if d.options.forceMask != nil {
perms = *d.options.forceMask
if forceMask != nil {
perms = *forceMask
}
applyDir = filepath.Join(layerDir, "dir")
if err := os.Mkdir(applyDir, perms); err != nil {
Expand Down Expand Up @@ -2225,6 +2231,7 @@ func (d *Driver) ApplyDiffWithDiffer(id, parent string, options *graphdriver.App
IgnoreChownErrors: d.options.ignoreChownErrors,
WhiteoutFormat: d.getWhiteoutFormat(),
InUserNS: unshare.IsRootless(),
ForceMask: forceMask,
}, &differOptions)

out.Target = applyDir
Expand Down
20 changes: 11 additions & 9 deletions pkg/chunked/storage_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ const (
maxNumberMissingChunks = 1024
autoMergePartsThreshold = 1024 // if the gap between two ranges is below this threshold, automatically merge them.
newFileFlags = (unix.O_CREAT | unix.O_TRUNC | unix.O_EXCL | unix.O_WRONLY)
containersOverrideXattr = "user.containers.override_stat"
bigDataKey = "zstd-chunked-manifest"
chunkedData = "zstd-chunked-data"
chunkedLayerDataKey = "zstd-chunked-layer-data"
Expand Down Expand Up @@ -1340,11 +1339,14 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
}

filesToWaitFor := 0
for i, r := range mergedEntries {
for i := range mergedEntries {
r := &mergedEntries[i]
if options.ForceMask != nil {
value := fmt.Sprintf("%d:%d:0%o", r.UID, r.GID, r.Mode&0o7777)
r.Xattrs[containersOverrideXattr] = base64.StdEncoding.EncodeToString([]byte(value))
r.Mode = int64(*options.ForceMask)
value := idtools.FormatContainersOverrideXattr(r.UID, r.GID, int(r.Mode))
if r.Xattrs == nil {
r.Xattrs = make(map[string]string)
}
r.Xattrs[idtools.ContainersOverrideXattr] = base64.StdEncoding.EncodeToString([]byte(value))
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit unclean to mutate the input here; I'd trust you if you said it was safe, but still.

In ostree we have something very similar to the ContainersOverrideXattr in the "bare-user" mode vs "bare" and the code paths in the backend are pretty distinct. Doing something similar here would want instead for e.g. setFileAttrs to take an override parameter or so perhaps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be safe because mergedEntries is an array of values (i.e. copies), created from scratch for this function. … ugh, except that a hash is a reference type, so AFAICS this might be affecting the c.toc field.

Ultimately, that is still safe because c.toc is written at most once, and read exactly once; but the number of assumptions is getting a bit large.

Copy link
Contributor

Choose a reason for hiding this comment

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

In Rust:

  • xattrs would likely be an Option<T> instead of a pointer, so the compiler would have made us check for its presence
  • We likely would have been passing a &TableOfContents or so, and the compiler would have errored out on an attempt to mutate it
  • And if we chose to pass instead &mut TableOfContents the compiler would verify that there weren't other threads (goroutines) that were concurrently reading it, etc.
  • But we can often do something better than mutation for something like this; Rust has efficient iterators over collections, and in this case instead of mutation it'd likely be better to leave the input read-only and instead pass impl Iterator<Item=Xattr> to the function that sets xattrs (instead of an array). It's efficient to do e.g. toc.xattrs.iter().chain(Xattr::new(idtools.CONTAINER_OVERRIDE_XATTR, value)) (although this would blend borrowed and owned values so we may need a Cow or so)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’d sign up to be a platinum member of the Go haters club, but that’s not really under discussion in this PR. (If you wanted to make a case for rewriting the stack in Rust, I’d not oppose that at all.)

I do agree that this would be safer with a deep copy, but then … Go doesn’t have a language-native deep copy operation either. So it’s either risk of bugs due to overwriting data (if it ever turned from single use into multiple-use) or risk of bugs due to missing a field copy (if we ever added a field), and to me that feels like a wash, up to @giuseppe .

}

mode := os.FileMode(r.Mode)
Expand Down Expand Up @@ -1393,7 +1395,7 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
return err
}
defer file.Close()
if err := setFileAttrs(dirfd, file, mode, &r, options, false); err != nil {
if err := setFileAttrs(dirfd, file, mode, r, options, false); err != nil {
return err
}
return nil
Expand All @@ -1408,7 +1410,7 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
if r.Name == "" || r.Name == "." {
output.RootDirMode = &mode
}
if err := safeMkdir(dirfd, mode, r.Name, &r, options); err != nil {
if err := safeMkdir(dirfd, mode, r.Name, r, options); err != nil {
return output, err
}
continue
Expand All @@ -1422,12 +1424,12 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
dest: dest,
dirfd: dirfd,
mode: mode,
metadata: &r,
metadata: r,
})
continue

case tar.TypeSymlink:
if err := safeSymlink(dirfd, mode, &r, options); err != nil {
if err := safeSymlink(dirfd, mode, r, options); err != nil {
return output, err
}
continue
Expand Down
8 changes: 7 additions & 1 deletion pkg/idtools/idtools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,12 @@ type Stat struct {
Mode os.FileMode
}

// FormatContainersOverrideXattr will format the given uid, gid, and mode into a string
// that can be used as the value for the ContainersOverrideXattr xattr.
func FormatContainersOverrideXattr(uid, gid, mode int) string {
return fmt.Sprintf("%d:%d:0%o", uid, gid, mode&0o7777)
}

// GetContainersOverrideXattr will get and decode ContainersOverrideXattr.
func GetContainersOverrideXattr(path string) (Stat, error) {
var stat Stat
Expand Down Expand Up @@ -413,7 +419,7 @@ func GetContainersOverrideXattr(path string) (Stat, error) {

// SetContainersOverrideXattr will encode and set ContainersOverrideXattr.
func SetContainersOverrideXattr(path string, stat Stat) error {
value := fmt.Sprintf("%d:%d:0%o", stat.IDs.UID, stat.IDs.GID, stat.Mode)
value := FormatContainersOverrideXattr(stat.IDs.UID, stat.IDs.GID, int(stat.Mode))
return system.Lsetxattr(path, ContainersOverrideXattr, []byte(value), 0)
}

Expand Down