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.

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: handle creating root directory #2194

Merged

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented Dec 9, 2024

if the file name is the root directory, do not attempt to use the base component since it is the empty string.

Closes: #2191

@giuseppe giuseppe force-pushed the mkdirat-do-not-create-empty-dir branch from 5c1461d to 09f5dfc Compare December 9, 2024 16:36
pkg/chunked/filesystem_linux.go Outdated Show resolved Hide resolved
pkg/chunked/storage_linux.go Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the mkdirat-do-not-create-empty-dir branch from 09f5dfc to 541b348 Compare December 9, 2024 20:13
@giuseppe giuseppe force-pushed the mkdirat-do-not-create-empty-dir branch from 541b348 to 5882567 Compare December 9, 2024 21:37
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks! This is nice.

pkg/chunked/filesystem_linux.go Outdated Show resolved Hide resolved
pkg/chunked/filesystem_linux.go Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the mkdirat-do-not-create-empty-dir branch 2 times, most recently from ddaf701 to b3357df Compare December 10, 2024 09:31
pkg/chunked/storage_linux.go Outdated Show resolved Hide resolved
pkg/chunked/storage_linux_test.go Outdated Show resolved Hide resolved
pkg/chunked/filesystem_linux_test.go Show resolved Hide resolved
pkg/chunked/filesystem_linux.go Outdated Show resolved Hide resolved
pkg/chunked/filesystem_linux.go Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the mkdirat-do-not-create-empty-dir branch 3 times, most recently from af09f80 to 3383a19 Compare December 10, 2024 21:14
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I didn't do any deep review but looks sane to me.

pkg/chunked/filesystem_linux.go Show resolved Hide resolved
pkg/chunked/storage_linux.go Outdated Show resolved Hide resolved
func dumpNode(out io.Writer, added map[string]*internal.FileMetadata, links map[string]int, verityDigests map[string]string, entry *internal.FileMetadata) error {
path := sanitizeName(entry.Name)
path := internal.CleanAbsPath(entry.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, composefs wants these to be absolute paths, so this makes sense.

(That said, it's kind of tempting to lift that and support relative paths too just on the principle that relative paths are IMO more "canonical" to use in use cases like tar)

Copy link
Contributor

openshift-ci bot commented Dec 10, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/chunked/filesystem_linux_test.go Outdated Show resolved Hide resolved
pkg/chunked/internal/path.go Outdated Show resolved Hide resolved
the returned file object is used later, so fail immediately if it
could not be opened.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the mkdirat-do-not-create-empty-dir branch from 3383a19 to db90d12 Compare December 11, 2024 10:26
ihen the file name is the root directory, avoid using an empty string
or the ".." name to open the file.  The latter does not cause any
security issues or unexpected behavior, it is logically incorrect and
should be avoided.

Closes: containers#2191

Signed-off-by: Giuseppe Scrivano <[email protected]>
replacing the usage of a private function.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe giuseppe force-pushed the mkdirat-do-not-create-empty-dir branch from db90d12 to 8fa5ca4 Compare December 11, 2024 12:38
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!

@openshift-merge-bot openshift-merge-bot bot merged commit 73af2c6 into containers:main Dec 11, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[zstd:chunked, rootless] staging a partially-pulled layer: mkdirat /: no such file or directory
4 participants