Ignore changing /dev/null permissions if used in STDIO#289
Ignore changing /dev/null permissions if used in STDIO#289mrunalp merged 1 commit intoopencontainers:masterfrom
Conversation
crosbymichael
force-pushed
the
stdio-null
branch
from
627adbf to
4ee3966
Compare
|
Was just about to push a patch to address the same issue (https://github.com/gitido/runc/commit/cbd3fe7c2488c8f17f0b11de69bc80a8c374bedf), but took a different approach. Just wondering if changing permissions of any device file is desirable, not just /dev/null. |
|
@GitIdo what about when things are stat /dev/pts/1
File: ‘/dev/pts/1’
Size: 0 Blocks: 0 IO Block: 1024 character special file
Device: dh/13d Inode: 4 Links: 1 Device type: 88,1
Access: (0620/crw--w----) Uid: ( 1000/ michael) Gid: ( 5/ tty)
Access: 2015-09-22 12:32:39.794966534 -0700
Modify: 2015-09-22 12:32:39.794966534 -0700
Change: 2015-09-22 10:25:04.794966534 -0700 |
|
@crosbymichael that's a good question. Not sure we'd want to change ownership there either however - it could have some implications on the process owning that pts, couldn't it? |
|
@GitIdo i think we would in the case of pts devices because they need to match the perms of the process that will own them. |
|
@crosbymichael just wondering if there aren't any security implications of doing something like this. I realize that not chowning the pts might break the process in the container, but doing so might have some implications on the host. Question is if libcontainer should be the one taking that decision. |
|
@GitIdo it's ok because we make the pty and libcontainer owns it. |
|
@crosbymichael that's true for runc, but not necessarily for other users of libcontainer, no? |
|
LGTM |
|
@LK4D4 ahh, i guess we can build from source in the dockerfile then to fix this? |
|
@crosbymichael Yup, #287 |
|
@LK4D4 @crosbymichael I'm finishing a PR to build criu from source just now |
|
LGTM |
|
Are we waiting for the build to come back before merging? |
Whenever dev/null is used as one of the main processes STDIO, do not try to change the permissions on it via fchown because we should not do it in the first place and also this will fail if the container is supposed to be readonly. Signed-off-by: Michael Crosby <[email protected]>
crosbymichael
force-pushed
the
stdio-null
branch
from
4ee3966 to
219b6c9
Compare
|
i rebased on master so you can run the tests locally |
|
okay |
|
Test passed locally. Merging. |
Ignore changing /dev/null permissions if used in STDIO
MAINTAINERS: correct Vish's github account
6 participants
Whenever dev/null is used as one of the main processes STDIO, do not try
to change the permissions on it via fchown because we should not do it
in the first place and also this will fail if the container is supposed
to be readonly.
Signed-off-by: Michael Crosby [email protected]