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 fchown when inheriting io #1354

Merged
merged 1 commit into from
Mar 3, 2017
Merged

Conversation

crosbymichael
Copy link
Member

This is a fix for rootless containers and general io handling. The
higher level systems must preparte the IO for the container in the
detach case and make sure it is setup correctly for the container's
process.

Replacement for #1349

Fixes detached use case for #774

Signed-off-by: Michael Crosby [email protected]

@rh-atomic-bot
Copy link

215/267 passed on RHEL - Failed.
215/267 passed on CentOS - Failed.
216/266 passed on Fedora - Failed.
Log - https://aos-ci.s3.amazonaws.com/opencontainers/runc/runc-integration-tests-prs/282/fullresults.xml

tty.go Outdated
@@ -62,19 +61,10 @@ func createStdioPipes(p *libcontainer.Process, rootuid, rootgid int) (*tty, erro
return t, nil
}

func dupStdio(process *libcontainer.Process, rootuid, rootgid int) error {
func inheritStdio(process *libcontainer.Process, rootuid, rootgid int) error {
Copy link
Member

Choose a reason for hiding this comment

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

Can we also drop the rootuid and rootuid arguments?

@williammartin
Copy link

Functionality works for us here in Garden-land against master and the rootless branch. Thanks @crosbymichael

This is a fix for rootless containers and general io handling.  The
higher level systems must preparte the IO for the container in the
detach case and make sure it is setup correctly for the container's
process.

Signed-off-by: Michael Crosby <[email protected]>
@crosbymichael
Copy link
Member Author

@cyphar updated

@cyphar
Copy link
Member

cyphar commented Mar 3, 2017

After trying to run this under #774, I found that there's still two places where we're doing Fchown of stdio fds:

% git grep Fchown
libcontainer/init_linux.go:             if err := syscall.Fchown(int(fd), u.Uid, int(s.Gid)); err != nil {
libcontainer/process_linux.go:          if err := syscall.Fchown(int(fd), rootuid, rootgid); err != nil {

So if you create a root terminal sudo -E termite or sudo -E gnome-terminal and then do sudo -u cyphar ./runc --root /tmp/runc create ctr it will fail with EINVAL because it ends up trying to fchown the stdio to an unmapped user.

If you want I can fix this in #774 or we can figure out whether it's safe to delete that code too.

@crosbymichael
Copy link
Member Author

@cyphar i can look at that independently also

@cyphar
Copy link
Member

cyphar commented Mar 3, 2017

Reading over fixStdioPermissions it looks like the code isn't wrong but we should modify it so that it handles user namespaces properly (namely handle unmapped UIDs and GIDs).

@cyphar cyphar self-requested a review March 3, 2017 01:25
@hqhq
Copy link
Contributor

hqhq commented Mar 3, 2017

LGTM

Approved with PullApprove

1 similar comment
@dqminh
Copy link
Contributor

dqminh commented Mar 3, 2017

LGTM

Approved with PullApprove

@dqminh dqminh merged commit 291bf60 into opencontainers:master Mar 3, 2017
@crosbymichael crosbymichael deleted the dup-io branch March 27, 2017 17:42
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.

6 participants