Dircache should handle fd == 0 as a valid file descriptor

[slackner]

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

[rfjakob]

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.

[rfjakob]

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.

[slackner]

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);

[slackner]

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.

[slackner]

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.

[rfjakob]

Good idea.

[rfjakob]

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

[rfjakob]

Hmm, unless the Stdlib decided to close that fd at some time. That would be a problem.

[slackner]

Would it make a difference when we move it to the init() function (instead of main())?

[rfjakob]

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

[rfjakob]

Found who opens /dev/null: https://github.com/hanwen/go-fuse/blob/f520193c2228d18ac98d8d83792c470803f26be5/splice/splice.go#L55

[slackner]

can we somehow inject our own init function (e.g., as a separate package) and ensure that it is executed before anything else?

[rfjakob]

I think this is where the eventpoll fd comes from: https://github.com/golang/go/blob/35f4ec152b44ae5fc83aaf68e2eb3aa1a778e5cd/src/runtime/netpoll_epoll.go#L26

[slackner]

Do you know if there is actually any way to influence the init() order? I tried with:

http://ix.io/1xz9

but it doesn't seem to make a difference.

[slackner]

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?

[rfjakob]

http://ix.io/1xz9

Also tried that, did not make a difference, hanwen runs first (yes, maybe alphabetic)

[rfjakob]

Another problem: when we damonize, we overwrite fds 0,1,2. This means Go loses the eventpoll fd.

[rfjakob]

./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]'

[rfjakob]

Oh, correction, it's all good (4 -> 'anon_inode:[eventpoll]').
The re-exec on daemonization fixes the problem.

[slackner]

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?!

http://ix.io/1xzg

[slackner]

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

[slackner]

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.

[rfjakob]

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]'

[slackner]

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).

[rfjakob]

Fixed by 7e05e80