spawn_mount is unsafe, but no reason provided in docs

[vi]

Currently it is the only unsafe function in my program. Without it (wrapped in thread::spawn instead) user has to manually unmount the system after Ctrl+C'ing the prorgam (which is handled by chan_signal, so can just fuse::mount).

[vi]

Meanwhile, my first rust-fuse project: https://crates.io/crates/outoforderfs

[cuviper]

It's because thread_scoped is unsafe, which is because a leaked guard may lead to invalid memory accesses. Same reason that API was deprecated from the standard library.

I'm not sure this thread actually needs to be scoped though, especially if all the parameters are fully moved into it. Might be worth revisiting.

[vi]

How can I unmount (including forced-unmount using fusectl abort) without spawn_mount? Maybe there should also be a dedicated unmount function?

And how mem::forgetting the returned guard may compromise safety? I expect the filesystem just stays mounted until external unmount request, like with usual non-spawn mount.

[cuviper]

In this case, a leak is probably OK, but it deserves careful analysis. The general problem in the standard library involved something like passing a local reference into a scoped thread, then leaking the guard and leaving that scope, such that the thread now has a reference that's no longer valid.

If you're able to achieve your needs with a manual thread::spawn, that's a good sign of perhaps how spawn_mount ought to work.

[vi]

Yes, it works with manual thread::spawn + mount (avoid_unsafe branch), but after Ctrl+C I get Transport endpoint is not connected instead of cleanly unmounted filesystem.

As a workaround, there can be additional closure to fuse::mount, which is has the handle for unmounting and may be started in (or moved to) a separate thread. Something like:

fn mount_for_3_seconds() {
    ::fuse::mount(myfs, mountpoint, &[], |h| {
         ::std::thread::sleep(Duration::from_secs(3));
         // exiting cleans up `h`, hence unmounts the filesystem
    });
}

[cuviper]

I was wrong, leaking the guard in this case could be bad too. We're sending the caller's Filesystem to a new thread, but since this is generic we don't know what's in that type. It could have any kind of member with a limited lifetime. So if the guard is forgotten and we leave that lifetime, then that other thread now has a Filesystem with invalid members. Something like:

struct Foo<'a>(&'a i32);
impl<'a> Filesystem for Foo<'a> { /* ... */ }
fn foo() {
    {
        let x = 42;
        let guard = unsafe { fuse::spawn_mount(Foo(&x), "foo", &[]).unwrap() };
        std::mem::forget(guard);
    }
    /* oops, the thread is still running, but &x is dead! */
}

So marking this unsafe is your warning flag to be sure about this. Either you must not leak the guard, or your Filesystem must be 'static so a leak can't hurt anyway. I agree the docs really should explain these reasons for unsafety here.

This is also why std::thread::spawn can't be used, unless we were to impose Filesystem: 'static.

[vi]

Should it be OK if Filesystem is 'static, hence owns x instead of borrows one? Oh, now I read the last sentence in the comment above.

Maybe there should be a separate method like fuse::spawn_mount_safe that requires 'static?

[cuviper]

A static safe variant sounds fine to me, or a scope-like variant with a closure to continue in the current thread. (Like your suggested workaround, but the closure doesn't even need any parameters.) Or add both variants to support different use cases.

[vi]

In any case, docs about spawn_mount should link the scoped thread unsafety problem somewhere.

[reem]

If you use a safe scoped thread library like scoped-pool you could remove the unsafety here - here's an example of safely using scoped-pool to write spawn_mount externally: https://github.com/terminalcloud/tfs/blob/e9a1ce1/src/lib.rs#L461-L482

EDIT: Note this example calls fuse::channel::unmount directly (I made it pub in a small fork) but within the library this is easily done.

[rustonaut]

I would opt for adding a spawn_mount_safe/static variation.

Maybe I would even remove the current spawn_mount (both static and scoped) removing the thread-scoped dependency. I would consider this because I don't think calling Session::run in a different thread is in the responsibility of this library. And it is kind of trivial to to so with whatever method the library user sees fit (scoped-pool,thread-scoped, some_other_thread_library, etc.). The main benefits would be: one dependency less, a view lines a code less and a pitfall less for everyone who is not aware of the thread scoped problem (I mean not creating it is unsafe but having it around is, so the unsaftiness escapes the unsafe blog).

Or the more compromising variation of only having spawn_mount (+'static) and adding a example to the docs how to spawn it without 'static and scoped-pool. I guess this might be the best variation :smile:

BTW: thanks for this nice library

[asomers]

Whatever the final solution is, the docs should definitely tell the user what he must do to use spawn_mount safely.

[atorstling]

I took a stab at replacing thread-scoped with crossbeam: https://github.com/zargony/rust-fuse/pull/121 .

I think it's useful to keep the spawn_mount function, since it adds the automatic unmount functionality when the handle is dropped. As it stands right now, there is no easy way of aborting without spawn_mount.

[samuela]

So what's the right approach to supporting ctrl-c-able fuse mounts? I'm using fuse::mount right now. I could switch to fuse::spawn_mount but it sounds like that is questionably unsafe, and potentially deprecated. Is it possible to ever unmount with fuse::mount? And how should this be done when a process is getting killed?