Incompatibility of fh returned by open/opendir

[xnoreq]

The issue leads to segfault with WinFSP-FUSE.

It is described here:
@Nikratio
winfsp/sshfs-win#54 (comment)

[Nikratio]

Thanks for the report! I believe sshfs-win is different from SSHFS. If you think a similar problem exists in SSHFS, could you please provide some more information? Without a reliable way to reproduce this issue under Linux, I'm afraid there isn't much we can do.

[xnoreq]

sshfs-win is different from SSHFS, yes, but the same problem happens with stock SSHFS on cygwin.

In the linked comment there is a very specific description of the issue. Let me quote:

There is a clear incompatibility here. WinFsp-FUSE treats values returned in fh either by open or opendir (or create) as valid values that can be passed to fgetattr. However SSHFS seems to not like that.

Digging into the SSHFS code we find sshfs_open and sshfs_opendir. Notice that fi->fh is assigned different kinds of objects in the 2 functions:

• sshfs_open
• sshfs_opendir

I believe the correct action here is to take this to the libfuse and sshfs maintainer @ nikratio and have a discussion with him. While I would like to argue that open and opendir should be returning fh values from the same "object space" and therefore fgetattr should work on both kinds of fh values (and I certainly design my file systems this way), the inconvenient truth is that what libfuse and sshfs does is correct by default and I will likely have to change WinFsp-FUSE to avoid calling fgetattr on an fh returned by opendir.

[Nikratio]

Thank you for clarifying the issue!

SSHFS is indeed following documented behavior here (cf. http:https://libfuse.github.io/doxygen/structfuse__operations.html#a60144dbd1a893008d9112e949300eb77, http:https://libfuse.github.io/doxygen/structfuse__operations.html#a1813889bc5e6e0087a936b7abe8b923f). You could make a case that it would have been better design to allow either kind of filehandle, but at this point way too many applications depend on the current API to justify changing it.

In other words, I still don't think there's anything to do an the SSHFS side here. Am I missing something?

[xnoreq]

Sorry, I'm not an expert in either FUSE nor SSHFS and just tried to relay the information but I've looked into the code a bit.

Here's what I don't get: in SSHFS opendir sets fi->fh to a struct buffer*. readdir and releasedir correctly cast fi->fh to struct buffer*. The documentation also mentions fsyncdir but I could only find fsync in SSHFS code.

Any other operation including fsync will call get_sshfs_file(fi) if fi != NULL which casts fi->fh to struct sshfs_file* and any follow up access may be illegal.
This is exactly what happens with WinFSP/FUSE in getattr which causes the crashes.

If I understood correctly then SSHFS is written such that if it's a directory then fi is always assumed to be passed in as NULL?

If two different types are necessary wouldn't it be more correct to add a tag to the structure that can be used to identify whether it's a file or directory struct and handle either case properly for all operations that could potentially handle files and directories?

Wouldn't it also be better for performance if any fuse filesystem implementation didn't necessarily need to "re-open" a directory for getattr after potentially doing all that work already in opendir?
I'd assume it's quite common to open a directory and then reading its attributes, no?

[Nikratio]

This discussion is somewhat academic. SSHFS uses libfuse, and libfuse guarantees that fi->fh values returned by opendir will never be passed to getattr. We could debate the merits of this libfuse design decision, but this won't be changed becauses it would break backwards compatibility.

If someone wants to submit a pull request for SSHFS so that its getattr can work with both kinds of fi->fh value as a courtesy for other projects that re-use the source code in a different context, that's fine with me. But the current behavior is 100% correct.

[Nikratio]

I'm closing this bug report for now. Please note that this isn't meant to imply that you haven't found a valid bug or potential improvement - you most likely have and I'm grateful that you took the time to report it.

Unfortunately, this project does not currently have any active, regular contributors or developers. The current maintainer continues to apply pull requests and tries to make regular releases, but unfortunately has no capacity to do any development beyond addressing high-impact issues. Any other reports are therefore likely to languish unless they come together with a high-quality pull request. (The issue reporting template has been changed to point this out, but you may have submitted your report before the change).

This report unfortunately does not fall into either category, so with the current state of affairs it's unlikely that anyone is going to be able to do anything but this anytime soon, and I prefer to use the issue tracker as a tool to manage ongoing work (as opposed to a database of known/potential issues).