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

Don't try to realpath() on /proc/$pid/fd magic links #1208

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Contributor

I'm trying to do skopeo copy docker://quay.io/cgwalters/fcos oci-archive://proc/self/fd/5
where a higher level process passes the write end of a pipe().

We can't call realpath on a pipe, and actually in general I think
programs should avoid running realpath and just operate on what
they're passed directly. There is presumably some reason
we're doing this but it seems like it should work to pass
a pipe, so let's not error out.

I'm trying to do `skopeo copy docker://quay.io/cgwalters/fcos oci-archive://proc/self/fd/5`
where a higher level process passes the write end of a `pipe()`.

We can't call `realpath` on a pipe, and actually in general I think
programs should avoid running `realpath` and just operate on what
they're passed directly.  There is presumably some reason
we're doing this but it seems like it should work to pass
a pipe, so let's not error out.
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thanks, @cgwalters!

Currently it doesn't build (imported and not used: "fmt").

@giuseppe can you have a look? My guts tell me you may know an alternative trick to check for /proc magic.

@giuseppe
Copy link
Member

there are already few different places where we expect /proc to be a procfs so unless we care about procfs also at a different path, I think the patch is fine.
Do we need to care if there are more than 2 additional / in the prefix (e.g. dir://///////////////tmp/foo is a valid destination)?

// This function gets passed the URL // in addition to a potentially absolute
// path, i.e. we trim oci-archive:///path/to/foo.tar to ///path/to/foo.tar,
// relying on the kernel to trim the useless // but let's normalize it now.
if strings.HasPrefix(path, "///") {
Copy link
Member

@rhatdan rhatdan Apr 23, 2021

Choose a reason for hiding this comment

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

Should just loop until path does not begin with //

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing in here are URLs, URL syntax concepts are not applicable.

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.

Yeah, there is “some reason”: the paths are defined as keys to a policy (compare ociArchiveReference.PolicyConfiguration{Identity,Namespaces}), and that doesn’t work if various ways to refer to the same file don’t compare equal. In retrospect unnecessary, I suppose, but not something that we can just ignore.

I guess this condition makes sense, but it definitely needs some canonicalization (filepath.Abs + filepath.Clean) to make the policy work at least enough to be able to write an “anything goes in /proc/self/fd” policy or the like. The /proc comparison is also only valid after filepath.Abs, strictly speaking.

Should this be limited to Linux? It’s clearly incorrect on Windows, not sure about non-Linux Unixes.

@mtrmac
Copy link
Collaborator

mtrmac commented Apr 23, 2021

Also, this is a part of the policy enforcement codebase, keep this as close to 100% test coverage as possible, please. (Compare the pre-existing “Coverage:” comments.)

@cgwalters
Copy link
Contributor Author

OK in the end this patch isn't actually useful today. Let's discuss higher level approach in #1209 and if it turns out we want containers/image to gracefully handle file descriptors we can revisit this.

@cgwalters cgwalters closed this Apr 27, 2021
@cgwalters
Copy link
Contributor Author

I realized there's a simple workaround: instead of using a pipe(2) I can use a temporary mkfifo(2), i.e. by giving the pipe a name it means realpath() works (even though it doesn't make sense to resolve its name).

i.e. I can do:

$ tempdir=$(mktemp -d)
$ mkfifo ${tempdir}/p
$ skopeo copy docker://quay.io/example/image:latest docker-archive:${tempdir}/p

Then another process gets streaming output via cat ${tempdir}/p, and I can clean up the fifo afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants