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

Fix imaginator to fail if umask!=022. #449

Merged
merged 1 commit into from
May 23, 2018
Merged

Fix imaginator to fail if umask!=022. #449

merged 1 commit into from
May 23, 2018

Conversation

rgooch
Copy link
Contributor

@rgooch rgooch commented May 23, 2018

This relates to issue #445.

@@ -47,6 +47,10 @@ func main() {
os.Exit(1)
}
logger := serverlogger.New("")
if umask := syscall.Umask(022); umask != 022 {
Copy link
Contributor

Choose a reason for hiding this comment

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

syscall.Umask never fails but it returns the previous value. Why does it matter for imaginator what the old value was?

@rgooch
Copy link
Contributor Author

rgooch commented May 23, 2018

See the comment on the next line: since you can't cleanly and reliably set the umask in Go, it's simpler to fail if the umask isn't 022.

@masiulaniec
Copy link
Contributor

I don't understand why one can't cleanly and reliably set the umask in Go... Is this related to Linux's infamously posix non-compliant threads implementation?

Reading umask(2) man page I can see some words of warning about non-atomicity in multi-threaded programs when one only wants to read the umask value without changing it but that is not the case here.

@rgooch
Copy link
Contributor Author

rgooch commented May 23, 2018

syscall.Umask will only change the umask for the OS thread that the goroutine is running on, and any subsequent OS threads that are created from that thread or it's descendants. Other OS threads are not affected. The main function goroutine is not running on the only OS thread in the process, so you can't rely on being the first OS thread (the Go runtime often spins up a couple of OS threads before the main function is called).
Yes, Linux threads are not POSIX threads (the latter support process-wide semantics). The various system calls that mutate the state of the "process" often only mutate the thread. This is both surprising for people who are used to different semantics (a diminishing group considering that Linux pretty much won the Unix wars) but is also useful. The Linux threads implementation is much more efficient, which was a significant motivating factor for the Linux approach. There are posix_* functions in the C library which emulate the POSIX threads API, but the system calls don't go through that emulation. I'm not aware of a library that provides a full set of syscall wrappers to fully emulate the POSIX threads environment and behaviour. I personally wouldn't use that in general since there is a performance impact.

@masiulaniec
Copy link
Contributor

Indeed, I encountered this property of Linux before when trying to add sshd-style sandboxing to some Go software: setresuid doesn't have process-wide effect on Linux. Personally, whenever I get curious about Unix implementation details, my go-to kernel to study is the one from OpenBSD; I guess that makes me a member of a diminishing group :-)

I still don't understand the diff. If umask's immediate effect is only on the calling thread, then don't we also need something like a surrounding runtime.LockOSThread/runtime.UnlockOSThread to make sure that the newly set umask gets used for our file operations? Go does not guarantee any goroutine-to-thread affinity without these calls. I'm not seeing any relevant runtime.LockOSThread in cmd/imaginator.

@rgooch
Copy link
Contributor Author

rgooch commented May 23, 2018

Again, the call to syscall.Umask is to check the umask (and then later fail if umask != 022). Since there isn't a call to only check the umask, I have to pass in the desired value (which will change nothing if the umask is already 022).

@masiulaniec
Copy link
Contributor

Got it, thanks.

@rgooch rgooch deleted the tmp branch May 29, 2018 04: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.

None yet

2 participants