libcontainer: LockOSThread around Umask calls#3563
Conversation
haircommander
force-pushed
the
umask-runtime-lock
branch
from
1fed69e to
b43a546
Compare
haircommander
force-pushed
the
umask-runtime-lock
branch
from
b43a546 to
1a964a1
Compare
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
I just so happen to be working on a very similar change over in moby/moby. Locking the goroutine to a thread is not sufficient to prevent umask changes from leaking into other threads.
| runtime.LockOSThread() | ||
| defer runtime.UnlockOSThread() | ||
| oldMask := unix.Umask(mask) | ||
| defer unix.Umask(oldMask) | ||
| return f() |
There was a problem hiding this comment.
The umask is part of the "filesystem information" whose sharing is governed by the CLONE_FS flag. The Go runtime clones new threads with the CLONE_FS flag set so any call to unix.Umask() immediately affects all goroutines in the process. runtime.LockOSThread() alone is not enough to prevent the change from leaking to other threads.
Here's what would work:
| runtime.LockOSThread() | |
| defer runtime.UnlockOSThread() | |
| oldMask := unix.Umask(mask) | |
| defer unix.Umask(oldMask) | |
| return f() | |
| errC := make(chan error) | |
| go func() { | |
| runtime.LockOSThread() | |
| if err := unix.Unshare(unix.CLONE_FS); err != nil { | |
| errC <- err | |
| runtime.UnlockOSThread() | |
| return | |
| } | |
| unix.Umask(mask) | |
| errC <- f() | |
| }() | |
| return <-errC |
unix.Unshare(CLONE_FS) is an irreversible operation; the calling thread will never again have its umask, cwd or root directory shared with other threads. The thread will no longer be indistinguishable from other threads which run goroutines. Returning from the goroutine while locked to the thread signals to the Go runtime to terminate the thread (or wedge, if it's the main thread) for situations exactly like this one. (This article has some good information as well.) Just be aware that terminating threads can have some undesirable interactions with (syscall.SysProcAttr).Pdeathsig which need to be accounted for. It's straightforward enough to deal with once you know how what's happening and why, e.g.: moby/moby#44215.
There was a problem hiding this comment.
thanks for the information! updated as suggested
haircommander
force-pushed
the
umask-runtime-lock
branch
from
1a964a1 to
b956c01
Compare
umask operates on a thread basis, and is part "filesystem information". Without both locking the OS thread, and calling `unshare(CLONE_FS)`, the umask will leak to other threads in runc. Signed-off-by: Peter Hunt~ <pehunt@redhat.com>
haircommander
force-pushed
the
umask-runtime-lock
branch
from
b956c01 to
fd264d6
Compare
There was a problem hiding this comment.
Sorry, I wrote a review some time ago but never submitted it. In short, I think that we should just remove all usage of unix.Umask in our codebase and just explicitly set the modes of everything we create. The CLONE_NEWFS issue is enough of a problem that we should just avoid over-complicating it (burning a thread each time we have to call umask(2) is kind of insane) and just do the simple thing.
Umask is problematic for Go programs as it affects other goroutines (see [1] for more details). Instead of using it, let's just prop up with Chmod. Note this patch misses the MkdirAll call in createDeviceNode. Since the runtime spec does not say anything about creating intermediary directories for device nodes, let's assume that doing it via mkdir with the current umask set is sufficient (if not, we have to reimplement MkdirAll from scratch, with added call to os.Chmod). [1] opencontainers#3563 (comment) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
closing in favor of #4039 |
4 participants
umask operates on a thread basis, and we risk leaking the umask to other routines
if the go runtime interrupts.
Signed-off-by: Peter Hunt~ pehunt@redhat.com