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

libcontainer: skip chown of /dev/null caused by fd redirection #3707

Merged
merged 2 commits into from Feb 7, 2023

Conversation

Dzejrou
Copy link
Contributor

@Dzejrou Dzejrou commented Jan 20, 2023

In #3355 the check whether the STDIO file descriptors point to /dev/null was removed which can cause /dev/null to change ownership e.g. when using docker exec on a running container:

$ ls -l /dev/null
crw-rw-rw- 1 root root 1, 3 Aug 1 14:12 /dev/null
$ docker exec -u test 0ad6d3064e9d ls
$ ls -l /dev/null
crw-rw-rw- 1 test root 1, 3 Aug 1 14:12 /dev/null

This PR reintroduces that check and fixes the issue for me on runc v1.1.{3,4} (I did not find any information about this change being intentional, if it was feel free to close this PR).

Fixes: #3674

@Dzejrou
Copy link
Contributor Author

Dzejrou commented Jan 20, 2023

I think this may be fixing #3674, but the reporter there never provided reproduction steps.

@cyphar cyphar requested a review from kolyshkin February 2, 2023 04:13
@cyphar cyphar added the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Feb 2, 2023
@cyphar
Copy link
Member

cyphar commented Feb 2, 2023

LGTM, however we require a DCO for all commits (just do git commit --amend -s and force-push to add it).

@kolyshkin It seems we broke this in #3345.

@cyphar cyphar added this to the 1.1.5 milestone Feb 2, 2023
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

The change looks good to me. I spent some time last week trying to create a repro but couldn't.

Can you please change the commit message to point to a commit (rather that GitHub PR)? Otherwise LGTM

@Dzejrou
Copy link
Contributor Author

Dzejrou commented Feb 2, 2023

LGTM, however we require a DCO for all commits (just do git commit --amend -s and force-push to add it).

Done.

Can you please change the commit message to point to a commit (rather that GitHub PR)? Otherwise LGTM

Is this format of commit reference OK?

kolyshkin
kolyshkin previously approved these changes Feb 2, 2023
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin kolyshkin modified the milestones: 1.1.5, 1.2.0 Feb 2, 2023
@kolyshkin
Copy link
Contributor

kolyshkin commented Feb 2, 2023

This is a commit to the main branch thus the milestone should be set to 1.2.0.

Indeed we need to backport it to release-1.1 as soon as this is merged, thus the appropriate label.

@kolyshkin
Copy link
Contributor

Ah! I finally got a reproducer for this, currently being tested in #3720.

@kolyshkin
Copy link
Contributor

Can you please rebase and pick up 65c94b8 ?

Something like

wget https://github.com/opencontainers/runc/pull/3720/commits/65c94b899ed0c7c5bceeec5c515990c392894db8.patch
git am 65c94b899ed0c7c5bceeec5c515990c392894db8.patch

should work

@kolyshkin kolyshkin self-requested a review February 3, 2023 00:16
Dzejrou and others added 2 commits February 3, 2023 01:24
In 18c4760 (libct: fixStdioPermissions: skip chown if not needed)
the check whether the STDIO file descriptors point to /dev/null was
removed which can cause /dev/null to change ownership e.g. when using
docker exec on a running container:

$ ls -l /dev/null
crw-rw-rw- 1 root root 1, 3 Aug 1 14:12 /dev/null
$ docker exec -u test 0ad6d3064e9d ls
$ ls -l /dev/null
crw-rw-rw- 1 test root 1, 3 Aug 1 14:12 /dev/null

Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@Dzejrou
Copy link
Contributor Author

Dzejrou commented Feb 3, 2023

Can you please rebase and pick up 65c94b8 ?

Done, I hope.

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

@cyphar @AkihiroSuda @thaJeztah PTAL

(once this is in, we need to backport to 1.1 and release 1.1.5)

Copy link
Member

@thaJeztah thaJeztah 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!

@kolyshkin
Copy link
Contributor

1.1 backport: #3731

@kolyshkin kolyshkin removed the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Feb 9, 2023
@kolyshkin kolyshkin added the backport/done/1.1 A PR in main branch which was backported to release-1.1 label Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done/1.1 A PR in main branch which was backported to release-1.1 regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

permanent changes in ownership of /dev/null
4 participants