Dircache should handle fd == 0 as a valid file descriptor
slackner opened this issue · 25 comments
While rather unlikely in practice, a file descriptor of 0 is completely valid and should not be treated as an error [1]. Both open and Dup typically return the smallest available file descriptor and only negative values are errors. If the user starts gocryptfs with stdin (fd == 0) closed and triggers the dircache code fast enough (not sure if that is actually possible), this could lead to a panic / unexpected behavior (e.g., in the fd validation in Store).
[1] https://unix.stackexchange.com/questions/100611/aix-open-file-descriptor-is-zero
Well, check this out: https://play.golang.org/p/gG0FOnelmzR
Compile and run it with fds 0,1,2 closed:
./printloop 0<&- 1>&- 2>&-
Now let's look at `strace -p $(pgrep printloop):
write(2, "2019/01/05 13:06:56 log\n", 31) = -1 EBADF (Bad file descriptor)
write(1, "fmt\n", 10) = -1 EBADF (Bad file descriptor)
This means fmt.Printf and log.Printfs blindly write into fds 1 and 2. So if the user closes all fds, they will have a bigger problem than the panic, because log output will end up in random files that happen to get opened at fd 1 or 2.
Note that this can only happen with -nosyslog, as otherwise daemonize.go redirects 0, 1 and 2 to valid fds.
If the user only closes fd 0 and uses -nosyslog, I guess what you describe could really happen. On the other hand, assuming that the default value "0" may be a valid fd is a PITA and an opportunity to introduce bugs by forgetting to initialize to -1. Not sure what to do.
Hmm. I guess we should check at least fds 1,2 on startup, and exit with an error if they are closed, or open /dev/null. And while we are at it, we can do the same thing for 0.
What about adding code early in the startup path to make sure those fds are registered? I am also using something like this in my security critical projects (and Wine uses it aswell):
if ((null_fd = open( "/dev/null", O_RDWR )) < 0)
{
perror( "open error" );
return 1;
}
while (null_fd >= 0 && null_fd <= 3)
null_fd = dup( null_fd );
close(null_fd);
For reference, here is what Wine does:
https://source.winehq.org/git/wine.git/blob/HEAD:/server/request.c#l824
The code relies on the fact that open and dup always allocate the smallest available fd on most systems (for Linux, it is definitely the case). If we need /dev/null later anyway, we can of course also just leave the fd open.
Note that in your case, checking null_fd >= 0 && null_fd <= 2 would probably be sufficient. I used limit 3 in my code above since in this particular project fd 3 also has a special meaning.
Good idea.
Pushed as ad15ad9 .
Seems to work OK, the Go stdlib opens that eventpoll thing concurrently, but I think that's harmless as writes into the eventpoll fd will do nothing. What do you think?
$ ls -l /proc/$(pgrep gocryptfs)/fd
total 0
l-wx------. 1 jakob jakob 64 Jan 5 14:14 0 -> /dev/null
lrwx------. 1 jakob jakob 64 Jan 5 14:14 1 -> 'anon_inode:[eventpoll]'
lrwx------. 1 jakob jakob 64 Jan 5 14:14 2 -> /dev/null
Hmm, unless the Stdlib decided to close that fd at some time. That would be a problem.
Would it make a difference when we move it to the init() function (instead of main())?
Good idea, but looks the same. Also, I am opening /tmp/ensureStdFds instead of /dev/null, and, interestingly, somebody else seems to open /dev/null first:
ls -l /proc/$(pgrep gocryptfs)/fd
total 0
l-wx------. 1 jakob jakob 64 Jan 5 14:39 0 -> /dev/null
lrwx------. 1 jakob jakob 64 Jan 5 14:39 1 -> 'anon_inode:[eventpoll]'
lrwx------. 1 jakob jakob 64 Jan 5 14:39 2 -> /tmp/ensureStdFds
Found who opens /dev/null: https://github.com/hanwen/go-fuse/blob/f520193c2228d18ac98d8d83792c470803f26be5/splice/splice.go#L55
can we somehow inject our own init function (e.g., as a separate package) and ensure that it is executed before anything else?
I think this is where the eventpoll fd comes from: https://github.com/golang/go/blob/35f4ec152b44ae5fc83aaf68e2eb3aa1a778e5cd/src/runtime/netpoll_epoll.go#L26
Do you know if there is actually any way to influence the init() order? I tried with:
but it doesn't seem to make a difference.
Unverfied, but https://medium.com/@markbates/go-init-order-dafa89fcef22 suggests
As you can see the init functions are loaded in order by the filename.
Does this mean we have no influence since rfjakob > hanwen?
Also tried that, did not make a difference, hanwen runs first (yes, maybe alphabetic)
Another problem: when we damonize, we overwrite fds 0,1,2. This means Go loses the eventpoll fd.
./gocryptfs -passfile test.txt a b 0<&- 1>&- 2>&-
Aaand it's gone:
$ ls -l /proc/$(pgrep gocryptfs)/fd
total 0
lr-x------. 1 jakob jakob 64 Jan 5 15:27 0 -> /dev/null
l-wx------. 1 jakob jakob 64 Jan 5 15:27 1 -> 'pipe:[108343707]'
lrwx------. 1 jakob jakob 64 Jan 5 15:27 10 -> 'socket:[108343706]'
l-wx------. 1 jakob jakob 64 Jan 5 15:27 2 -> 'pipe:[108343707]'
l-wx------. 1 jakob jakob 64 Jan 5 15:27 3 -> /dev/null
lrwx------. 1 jakob jakob 64 Jan 5 15:27 4 -> 'anon_inode:[eventpoll]'
lrwx------. 1 jakob jakob 64 Jan 5 15:27 5 -> 'socket:[108343702]'
lrwx------. 1 jakob jakob 64 Jan 5 15:27 6 -> 'socket:[108343703]'
lrwx------. 1 jakob jakob 64 Jan 5 15:27 7 -> /dev/fuse
lrwx------. 1 jakob jakob 64 Jan 5 15:27 8 -> 'socket:[108343704]'
lrwx------. 1 jakob jakob 64 Jan 5 15:27 9 -> 'socket:[108343705]'
Oh, correction, it's all good (4 -> 'anon_inode:[eventpoll]').
The re-exec on daemonization fixes the problem.
It still feels a bit error-prone, and its a bit surprising that Go doesn't offer any clean way to handle init priorities or something like that. The following patch seems to work, but I have no idea why?!
strace output:
(ensureStdFds logic)
openat(AT_FDCWD, "/dev/null", O_RDWR) = 0
dup(0) = 3
close(3) = 0
[...]
(other libs and epoll stuff)
openat(AT_FDCWD, "/proc/sys/net/core/somaxconn", O_RDONLY|O_CLOEXEC) = 3
epoll_create1(EPOLL_CLOEXEC) = 4
[...]
(/dev/null opened by go-fuse)
openat(AT_FDCWD, "/dev/null", O_WRONLY) = 3
My interpretation would be, that cli_args.go is processed first (due to the filename ordering). The imports are then executed in the order they are listed. But this is just a wild guess.
Well, awesome!
$ ls -l /proc/$(pgrep gocryptfs)/fd
total 0
lrwx------. 1 jakob jakob 64 Jan 5 15:47 0 -> /dev/null
lrwx------. 1 jakob jakob 64 Jan 5 15:47 1 -> /dev/null
lrwx------. 1 jakob jakob 64 Jan 5 15:47 2 -> /dev/null
l-wx------. 1 jakob jakob 64 Jan 5 15:47 3 -> /dev/null
lrwx------. 1 jakob jakob 64 Jan 5 15:47 4 -> 'anon_inode:[eventpoll]'
Awesome! I've created a pull request (but feel free to adjust the commit if you would like to call the package differently or change anything else).