Safety: Make Channel/Session safer by clarifying ownership

[wmanley]

This splits Channel in two. Channel now only encapsulates reading from the FUSE device. It is no longer responsible for unmounting the filesystem. Unmounting falls to a new, backend-specific, struct Mount. Channel and ChannelSender now have shared ownership (via Arc) of the FUSE device, and it's closed when it's no longer needed.

The Mount belongs to session. This way the Session can drop the mount without also closing the fuse device file. This is important as we don't want to close an FD that is still in use as we can cause the equivalent of use-after-free bugs†. When a filesystem is unmounted the devices return ENODEV, so should naturally exit as well in an orderly fashion and without races.

Ownership is now a lot more straightforward. In particular the libfuse3 fuse_session lifecycle was a little confused before, while now we use RAII in a straightforward fashion to ensure things are cleaned up correctly, and not cleaned up twice. The previous implementation had a bug where it would never be freed because fuse_session_unmount would only be called on null pointers.

The fuse device objects are plain std::fs::Files with ownership over the underlying FD. Note: although they are Files, we still implement our own reading and writing via libc::read and libc::writev due to concerns about the guarantees that std::fs::File's AsyncRead and AsyncWrite provides. We can still revisit this decison in the future.

Generally I've followed the principle that it's a good idea to encapsulate an FD as soon as we can for greater confidence that it will get cleaned up correctly.

This contains API additions, but no API breakage.

TODO:

• [x] Re-instate polling fd before attempting unmount

[cberner]

Cool! I'll take a look this weekend, if not sooner

[ianoc]

This is important as we don't
want to close an FD that is still in use as we can cause the equivalent
of use-after-free bugs†

could you expand on this? I'm not sure there are use after free concerns, if the socket is closed then all the calls will fail. If we are concerned about using a re-allocated FD we probably should address that directly. (fwiw, this code and the master code is calling umount, which isn't done in libfuse or the go impl that i can work out without the autounmount flag being present, which we should probably be following too).

[wmanley]

If we are concerned about using a re-allocated FD we probably should address that directly.

Yes, this is the concern. For example: you'd be pretty miffed if in your multi-threaded app you open SQLite in one thread while closing your FUSE device and you ended up with a corrupt database due to the race condition. This is addressed in f3a5499.

[cberner]

This is important as we don't
want to close an FD that is still in use as we can cause the equivalent
of use-after-free bugs†

could you expand on this? I'm not sure there are use after free concerns, if the socket is closed then all the calls will fail. If we are concerned about using a re-allocated FD we probably should address that directly. (fwiw, this code and the master code is calling umount, which isn't done in libfuse or the go impl that i can work out without the autounmount flag being present, which we should probably be following too).

Good point about not unmounting unless AutoUnmount is present. The calls to unmount are from the original fuse-rs code, but I believe you're right that libfuse doesn't do that, and so we shouldn't either

[wmanley]

Good point about not unmounting unless AutoUnmount is present. The calls to unmount are from the original fuse-rs code, but I believe you're right that libfuse doesn't do that, and so we shouldn't either

I'm not familiar with libfuse. What's the use-case for having a fuse filesystem mounted that doesn't work? IOW why would not want auto-unmount?

Looking at the man page:

   libfuse-specific mount options:
       These following options are not actually passed to the kernel but
       interpreted by libfuse. They can be specified for all filesystems
       that use libfuse:

       auto_unmount
              This option enables automatic release of the mountpoint if
              filesystem terminates for any reason. Normally the
              filesystem is responsible for releasing the mountpoint,
              which means that the mountpoint becomes inaccessible if
              the filesystem terminates without first unmounting.

The way I read "normally the filesystem is responsible for releasing the mountpoint" is that filesystems should be calling unmount explicitly, but the auto_unmount option is for enabling the trick where an external process is responsible for the unmount when that socket closes. I guess that helps cleanup when a fuse filesystem process is forcibly killed or dies unexpectedly. I don't think it means that it's desirable to leave a filesystem mounted if it is broken - it just controls the mechanism by which cleanup takes place.

As I say, I'm not familiar with libfuse, so I could easily be missing something here.

As it stands this PR preserves (or attempts to preserve) the existing semantics. If we want to fix/change the handling of automount IMO that deserves its own PR.

[wmanley]

test_mount_unmount hangs in CI on MacOS

I'm not sure what to do here. I don't have a Mac to debug this on. If someone could tell me where it's blocked (with strace) then I can investigate further.

[cberner]

I don't use a Mac for development either. The last time I debugged a Mac issue, I added debug code to the Mac CI and print to the console. If you want an interactive session, you could also look at EC2. I think they offer Mac instances now

[wmanley]

From my investigation: it appears that unmount fails on MacOS. This causes this test to fail. The test then hangs when attempting to delete the temporary directory becuase it tries to access the mountpoint.

See logs here: https://github.com/cberner/fuser/pull/128/checks?check_run_id=2088183919#step:10:65

From reading the code to osxfuse I believe that unmounting on MacOS relies on autounmount - i.e. there's a control socket to the child mount_macfuse process and when that is closed the FS is unmounted by the child. Unfortunately from reading the code it seems that this socket FD is leaked. So it means that the FS will be unmounted when the process dies, but not before. Because we don't know which socket it is we can't close it ourselves to cause the mount to end before the process does, if the call to umount fails.

One possibility would be to pass MNT_FORCE to umount. This is what we do in fuse_unmount_pure on MacOS.

What I'll do for now is to fix the test such that it leaks the tempdir on failure, so we don't get a hang. Also I'll skip the unmount assertion on non-linux as on Linux it should always succeed (thanks to MNT_DETACH) while it can fail on other systems.

[wmanley]

The test "passes" now, so CI is green.

@cberner: I've rebased on master to resolve conflicts, but really only the last 4 commits are new since last time you reviewed.

[cberner]

Great, I'll take a look!

[cberner]

@wmanley looks good to me! Could you rebase, there's a conflict? Then I'll merge this

[wmanley]

@wmanley looks good to me! Could you rebase, there's a conflict? Then I'll merge this

Thanks for the review. Rebased.

[cberner]

Merged. Thanks!