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

macfuse+pthreads = race condition in mount and deadlock in fuse_unmount #5

Open
anatol opened this issue May 17, 2011 · 15 comments
Open

Comments

@anatol
Copy link
Contributor

anatol commented May 17, 2011

Hi,

ok finally I created a reduced testcase that shows issues with macfuse+pthreads. macfuse works fine if fuse_loop() is in the main thread. But when we want to run it in a separate thread and access filesystem from another thread - we have issues.

The test case is at https://gist.github.com/976896 See the commends at the bottom of the file that briefly describe the issues.

@anatol
Copy link
Contributor Author

anatol commented May 17, 2011

@anatol
Copy link
Contributor Author

anatol commented May 17, 2011

I also sent a message to fuse-devel to clarify the specification of fuse_new() function

http://sourceforge.net/mailarchive/message.php?msg_id=27515148

@bfleischer
Copy link
Owner

At least in some cases (if (! init_backgrounded()) in fuse_mount_kern() in mount_bsd.c) the function fuse_new() might return before the filesystem is actually mounted on BSD, too. So I'm not sure if there is a standard. But I hope you get an reply from fuse-devel to be sure.

@anatol
Copy link
Contributor Author

anatol commented May 18, 2011

You might be interested in the commit that added init_background
property to the bsd kernel module
http://mercurial.creo.hu/repos/fuse4bsd-hg/index.cgi/rev/af0b89b9e9f2

At least in some cases (if (! init_backgrounded())
This line is needed for backward compatibility, when newer libfuse
works with old bsd kernel that does not support init_background
property. It is safe to assume that now on bsd fuse_mount runs in
foreground.

So I'm not sure if there is a standard.
I am trying to clarify whether "block thread in fuse_new()" is a part
of fuse "standard" (aka whether such behavior is implemented
intentionally).
http://sourceforge.net/mailarchive/forum.php?thread_name=BANLkTikdSeR%2B9WV5kRJr3UdmC%2BFEnrB1Dw%40mail.gmail.com&forum_name=fuse-devel

@anatol
Copy link
Contributor Author

anatol commented May 18, 2011

The author of fuse clarified situation:

"It blocks access to the filesystem after fuse_mount() succeeds up until
the INIT reply is received by the kernel. That's intentional, the
filesystem daemon goes into background only after mount(2) has
succeeded. This means it is possible to rely on this getting a listing
of the filesystem:

fusexmp /tmp/mnt; ls /tmp/mnt

"

http://sourceforge.net/mailarchive/forum.php?thread_name=87liy4gid4.fsf%40tucsk.pomaz.szeredi.hu&forum_name=fuse-devel

So this is a bug in macfuse that should be fixed.

@bfleischer
Copy link
Owner

Without having spend much time looking into this, I think we need to remove the double fork in fuse_mount_core. Would you agree or did I overlook something?

@anatol
Copy link
Contributor Author

anatol commented May 18, 2011

I tried and it did not help: fuse_mount thread is blocked until you
call fuse_loop(). You can see it by modifying the testcase with added
sleep(10) after every fuse function, and running "ps -axf | grep fuse"
in another terminal.

So the real filesystem mount in the macfuse happens on fuse_loop(),
until then the mount point is just a regular directory.

@anatol
Copy link
Contributor Author

anatol commented May 19, 2011

I enabled trace and msleep log and here is the log. The umount deadlock happened at 17:57:24

May 18 17:56:53 kernel[0]: fuse_devices_start
May 18 17:56:53 kernel[0]: MacFUSE: starting (version 2.2.0, May 18 2011, 17:55:29)
May 18 17:57:24 kernel[0]: 0): fuse_insert_message@693 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x94d0b30): fticket_wait_answer@199 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x94d0b30): fticket_wait_answer@199 by 7003
May 18 17:57:24 kernel[0]: 0: msleep(0xe545e00, fu_ans)
May 18 17:57:24 kernel[0]: 1: msleep(0xb152000, fu_msg)
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x84f28a0): fuse_device_read@340 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x84f28a0): fuse_device_read@340 by 7003
May 18 17:57:24 kernel[0]: fuse_device_write
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x94d60f0): fuse_device_write@445 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x94d60f0): fuse_device_write@445 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x94d60f0): fuse_device_write@455 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x94d60f0): fuse_device_write@455 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x94d0b30): fuse_standard_handler@943 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x94d0b30): fuse_standard_handler@943 by 7003
May 18 17:57:24 kernel[0]: 1: wakeup(0xe545e00)
May 18 17:57:24 kernel[0]: 0: wakeup(0xe545e00)
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x94d0b30): fuse_standard_handler@953 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x94d0b30): fuse_standard_handler@953 by 7003
May 18 17:57:24 kernel[0]: fuse_device_read
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x84f28a0): fuse_device_read@313 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x84f28a0): fuse_device_read@313 by 7003
May 18 17:57:24 kernel[0]: 0: msleep(0xb152000, fu_msg)
May 18 17:57:24 kernel[0]: 1: msleep(0xe545e00, fu_ans)
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x94d0b30): fticket_wait_answer@328 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x94d0b30): fticket_wait_answer@328 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_drop@619 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_drop@619 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@626 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@626 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_drop@628 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_drop@628 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@639 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@639 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_fetch@578 by 17
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_fetch@578 by 17
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_fetch@604 by 17
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_fetch@604 by 17
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x94d60f0): fuse_insert_callback@669 by 17
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x94d60f0): fuse_insert_callback@669 by 17
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x94d60f0): fuse_insert_callback@671 by 17
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x94d60f0): fuse_insert_callback@671 by 17
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x84f28a0): fuse_insert_message@687 by 17
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x84f28a0): fuse_insert_message@687 by 17
May 18 17:57:24 kernel[0]: 1: wakeup_one(0xb152000)
May 18 17:57:24 kernel[0]: 0: wakeup_one(0xb152000)
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x84f28a0): fuse_insert_message@693 by 17
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x84f28a0): fuse_insert_message@693 by 17
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x94d0b30): fticket_wait_answer@199 by 17
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x94d0b30): fticket_wait_answer@199 by 17
May 18 17:57:24 kernel[0]: 0: msleep(0xe545e00, fu_ans)
May 18 17:57:24 kernel[0]: 1: msleep(0xb152000, fu_msg)
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x84f28a0): fuse_device_read@340 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x84f28a0): fuse_device_read@340 by 7003
May 18 17:57:24 kernel[0]: fuse_device_ioctl
May 18 17:57:24 kernel[0]: 0: lck_mtx_lock(0x9497630): fuse_device_ioctl@608 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_lock(0x9497630): fuse_device_ioctl@608 by 7003
May 18 17:57:24 kernel[0]: 1: lck_mtx_unlock(0x9497630): fuse_device_ioctl@683 by 7003
May 18 17:57:24 kernel[0]: 0: lck_mtx_unlock(0x9497630): fuse_device_ioctl@683 by 7003
May 18 17:58:23 KernelEventAgent[44]: tid 00000000 received event(s) VQ_DEAD (32)
May 18 17:58:24 kernel[0]: nsw@1061 by 24
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x94d0b30): fdisp_wait_answ@1065 by 24
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x94d0b30): fdisp_wait_answ@1065 by 24
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_drop@619 by 24
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_drop@619 by 24
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@626 by 24
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@626 by 24
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_drop@628 by 24
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_drop@628 by 24
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@639 by 24
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@639 by 24
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_fetch@578 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_fetch@578 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_fetch@604 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_fetch@604 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x94d0b30): fticket_wait_answer@199 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x94d0b30): fticket_wait_answer@199 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x94d0b30): fticket_wait_answer@328 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x94d0b30): fticket_wait_answer@328 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x94d0b30): fdisp_wait_answ@1061 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x94d0b30): fdisp_wait_answ@1061 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x94d0b30): fdisp_wait_answ@1065 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x94d0b30): fdisp_wait_answ@1065 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_drop@619 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_drop@619 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@626 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@626 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x78c5580): fuse_ticket_drop@628 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x78c5580): fuse_ticket_drop@628 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@639 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x78c5580): fuse_ticket_drop@639 by 7003
May 18 17:58:24 kernel[0]: fuse_vfsop_unmount: Calling vflush(mp, fuse_rootvp, flags=0x0);
May 18 17:58:24 kernel[0]: fuse_vfsop_unmount: Done.
May 18 17:58:24 kernel[0]: fuse_vfsop_unmount: Calling vnode_rele(fuse_rootp);
May 18 17:58:24 kernel[0]: fuse_vfsop_unmount: Done.
May 18 17:58:24 kernel[0]: fuse_vfsop_unmount: Calling vflush(mp, NULLVP, FORCECLOSE);
May 18 17:58:24 kernel[0]: fuse_vfsop_unmount: Done.
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x9497630): fuse_device_lock@65 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x9497630): fuse_device_lock@65 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x9497630): fuse_device_unlock@72 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x9497630): fuse_device_unlock@72 by 7003
May 18 17:58:24 kernel[0]: fuse_device_ioctl
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x9497630): fuse_device_ioctl@608 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x9497630): fuse_device_ioctl@608 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x84f28a0): fdata_set_dead@496 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x84f28a0): fdata_set_dead@496 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x84f28a0): fdata_set_dead@498 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x84f28a0): fdata_set_dead@498 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x78a52c0): fuse_device_ioctl@632 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x78a52c0): fuse_device_ioctl@632 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x78a52c0): fuse_device_ioctl@634 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x78a52c0): fuse_device_ioctl@634 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x9497630): fuse_device_ioctl@683 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x9497630): fuse_device_ioctl@683 by 7003
May 18 17:58:24 kernel[0]: fuse_device_close
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x84f28a0): fdata_set_dead@496 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x84f28a0): fdata_set_dead@496 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x84f28a0): fdata_set_dead@498 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x84f28a0): fdata_set_dead@498 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x9497630): fuse_device_close@238 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x9497630): fuse_device_close@238 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x94d60f0): fuse_device_close@242 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x94d60f0): fuse_device_close@242 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x9497630): fuse_device_close@276 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x9497630): fuse_device_close@276 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_lock(0x94e9030): fuse_device_close@278 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_lock(0x94e9030): fuse_device_close@278 by 7003
May 18 17:58:24 kernel[0]: 1: lck_mtx_unlock(0x94e9030): fuse_device_close@288 by 7003
May 18 17:58:24 kernel[0]: 0: lck_mtx_unlock(0x94e9030): fuse_device_close@288 by 7003

Continue investigation...

@anatol
Copy link
Contributor Author

anatol commented May 19, 2011

Ok, I fixed the second "deadlock on unmount" issue. Here is the story:

I tried to output more logs from the kext but there were no any useful information, all locks looked ok. So I decided to run dtruss over the testcase and found that the lock happens on "getattr()" function, not the unmount. So I looked into fuse_kern_unmount() from mount_darwin.c and yes - fuse_unmount() tries to dereference the mountpoint filename using realpath() and it calls getattr() first.

Jeeez it looks like nobody used fuse_exit() with fuse_unmount(), everyone is just using fuse_main().

But wait, how the fuse_main() deinitializes? Here is the code https://github.com/anatol/fuse/blob/339ee2d259837f31d3da955a30595c0a8f91c977/lib/fuse_darwin.c So macfuse has 2 different ways to deinitialize itself. Using fuse_exit()/fuse_unmount() that has the deadlock, and calling( fuse_exit_handler_internal_np() (this is what called if you use fuse_main()). If you look into those two functions you'll find a bunch of duplicated code. And BTW I also see that schedule_umount() function does not reap its children processes, so this function creates a bunch of zombies.

Ok, the deadlock is fixed. Please test it. anatol/fuse@339ee2d and if it is ok - pull these changes. I am going to look into de-initialization logic and clean it. The code should be as close as possible to the upstream fuse version.

@anatol
Copy link
Contributor Author

anatol commented May 19, 2011

So I want to cleanup the deinialization logic first. After that I'll look into the first issue. It will probably take more time and will require some amount of kext changes.

@bfleischer
Copy link
Owner

Ok, the deadlock is fixed. Please test it. anatol/fuse@339ee2d and if it is ok - pull these changes. I am going to look into de-initialization logic and clean it. The code should be as close as possible to the upstream fuse version.

I did some preliminary tests and everything seems to work fine. Do you know any reason why Amit used realpath to resolve the mountpoint? Using a relative path containing symlinks works fine. As far as I can tell there seems to be no need to use the canonicalized absolute pathname of the mountpoint to unmount it.

Could this be a limitation on previous versions of Mac OS X? The man pages for unmount gave no indication that this could be the case.

@anatol
Copy link
Contributor Author

anatol commented May 19, 2011

Hi

On Thu, May 19, 2011 at 4:36 PM, bfleischer
reply@reply.github.com
wrote:

Ok, the deadlock is fixed. Please test it. anatol/fuse@339ee2d and if it is ok - pull these changes. I am going to look into de-initialization logic and clean it. The code should be as close as possible to the upstream fuse version.

I did some preliminary tests and everything seems to work fine. Do you know any reason why Amit used realpath to resolve the mountpoint? Using a relative path containing symlinks works fine. As far as I can tell there seems to be no need to use the canonicalized absolute pathname of the mountpoint to unmount it.

I do not see any reason. fuse_main() function already dereferenced the
path (see fuse_helper_opt_proc()) there is no reason to dereference it
once again.

Could this be a limitation on previous versions of Mac OS X? The man pages for unmount gave no indication that this could be the case.

I think this is just a mistake. The path is already dereferenced. As
of mount() - I would expect that it fails on symlink (or at least
behavior is undefined).

@bfleischer
Copy link
Owner

I do not see any reason. fuse_main() function already dereferenced the
path (see fuse_helper_opt_proc()) there is no reason to dereference it
once again.

Are you sure? When you invoke the function fuse_unmount you specify the mountpoint as the first argument. fuse_unmount performs no checks on the pathname and invokes fuse_kern_unmount which will use the syscall unmount. the char *mountpoint has never been touched and contains whatever you specified.

Could this be a limitation on previous versions of Mac OS X? The man pages for unmount gave no indication that this could be the case.

I think this is just a mistake. The path is already dereferenced. As
of mount() - I would expect that it fails on symlink (or at least
behavior is undefined).

Works for me with symlinks. But on Solaris the unmount system call seems to fail if mountpoint is not an absolute pathname.

There might be an additional problem: mount_hash
mount_hash stores every mounted fuse filesystem with mountpoint as key. So if you use a different path (e.g. a symlink or a relative path) when calling function fuse_unmount the mointpoint won't be removed from the hash and the internal counter mount_count will be 1 too high. Worse, if you try to remount the filesystem at the original mountpoint (still stored in the hash) libfuse will tell you that there is already a filesystem mounted at this particular mountpoint. The mount will fail. Hashing the absolute pathname would prevent this.

Have a look at the gist https://gist.github.com/de320bcf06ec6be8069c
The gist contains a modified mount_darwin.c (two printfs for mount_count added) and the modified file issue.c (use symlink in fuse_unmount function call)

Replace mount_darwin.c and compile libfuse
gcc -Wall pkg-config fuse --cflags --libs issue.c -o issue
mkdir mountpoint
ln -s mountpoint link
./issue

If we agree that the filesystem has to use the same pathname for mountpoint to mount and unmount then we would be fine. I don't see such a limitation in mount.c. What do you think?

@anatol
Copy link
Contributor Author

anatol commented May 20, 2011

Hi

On Thu, May 19, 2011 at 5:55 PM, bfleischer
reply@reply.github.com
wrote:

I do not see any reason. fuse_main() function already dereferenced the
path (see fuse_helper_opt_proc()) there is no reason to dereference it
once again.

Are you sure? When you invoke the function fuse_unmount you specify the mountpoint as the first argument. fuse_unmount performs no checks on the pathname and invokes fuse_kern_unmount which will use the syscall unmount. the char *mountpoint has never been touched and contains whatever you specified.

Sorry I've should be clearer. If you use fuse_main() then the path is
dereferenced for you. It means that this change is fully backward
compatible. And nobody uses fuse_mount()/fuse_loop()/fuse_unmount()
(it does not work with the Amit code because of issue #5 item 2).

And the new developers who'll be using fuse_loop()/fuse_unmount() will
be needed to dereference path before using. I think this is reasonable
requirement.

Could this be a limitation on previous versions of Mac OS X? The man pages for unmount gave no indication that this could be the case.

I think this is just a mistake. The path is already dereferenced. As
of mount() - I would expect that it fails on symlink (or at least
behavior is undefined).

Works for me with symlinks. But on Solaris the unmount system call seems to fail if mountpoint is not an absolute pathname.

There might be an additional problem: mount_hash
mount_hash stores every mounted fuse filesystem with mountpoint as key. So if you use a different path (e.g. a symlink or a relative path) when calling function fuse_unmount the mointpoint won't be removed from the hash and the internal counter mount_count will be 1 too high. Worse, if you try to remount the filesystem at the original mountpoint (still stored in the hash) libfuse will tell you that there is already a filesystem mounted at this particular mountpoint. The mount will fail. Hashing the absolute pathname would prevent this.

Have a look at the gist https://gist.github.com/de320bcf06ec6be8069c
The gist contains a modified mount_darwin.c (two printfs for mount_count added) and the modified file issue.c (use symlink in fuse_unmount function call)

Replace mount_darwin.c and compile libfuse
gcc -Wall pkg-config fuse --cflags --libs issue.c -o issue
mkdir mountpoint
ln -s mountpoint link
./issue

If we agree that the filesystem has to use the same pathname for mountpoint to mount and unmount then we would be fine. I don't see such a limitation in mount.c. What do you think?

Mounting one path and then unmounting via symlink does not sound like
a right thing to me.

As of fuse_hash and whole fuse_darwin.c - this is total pile of mess
that should be removed. The only thing why fuse_hash is needed is ...
synchronous mount and unmount. Which is the issue #5 item 1. After I
implement it (I hope that it'll take not more than a few weeks) the
fuse_hash will be removed. There should be only 1 way to de-initialize
fuse session. And that code (as well as library behavior) should be
kept as close as possible to the upstream Linux code.

@bfleischer
Copy link
Owner

I do not see any reason. fuse_main() function already dereferenced the
path (see fuse_helper_opt_proc()) there is no reason to dereference it
once again.

Are you sure? When you invoke the function fuse_unmount you specify the mountpoint as the first argument. fuse_unmount performs no checks on the pathname and invokes fuse_kern_unmount which will use the syscall unmount. the char *mountpoint has never been touched and contains whatever you specified.

Sorry I've should be clearer. If you use fuse_main() then the path is
dereferenced for you. It means that this change is fully backward
compatible. And nobody uses fuse_mount()/fuse_loop()/fuse_unmount()
(it does not work with the Amit code because of issue #5 item 2).

And the new developers who'll be using fuse_loop()/fuse_unmount() will
be needed to dereference path before using. I think this is reasonable
requirement.

Makes sense.

As of fuse_hash and whole fuse_darwin.c - this is total pile of mess
that should be removed. The only thing why fuse_hash is needed is ...
synchronous mount and unmount. Which is the issue #5 item 1. After I
implement it (I hope that it'll take not more than a few weeks) the
fuse_hash will be removed. There should be only 1 way to de-initialize
fuse session. And that code (as well as library behavior) should be
kept as close as possible to the upstream Linux code.

I could not agree more. It would be bad if MacFUSE (or the new fork) would behave differently than the official Linux version. Cleaning up that mess will make future development much easier and should have the highest priority right now.

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

No branches or pull requests

2 participants