fix: replace assert(offset==0) with early return in readdir#371
Conversation
There was a problem hiding this comment.
Why assert that offset must be 0, and then handle the case where it isn't?
There was a problem hiding this comment.
Because previously sshfs uses buf_get_entries (sshfs.c: 985) with offset=0, and assert(offset=0) is then used in sftp_readdir_async / sftp_readdir_sync for bug detection. In this process, all directory entries are returned from the server within one step. If the offset is non-zero, something might be wrong. See the notes in fuse.h:
/** Read directory
*
* The filesystem may choose between two modes of operation:
*
* 1) The readdir implementation ignores the offset parameter, and
* passes zero to the filler function's offset. The filler
* function will not return '1' (unless an error happens), so the
* whole directory is read in a single readdir operation.
*
* 2) The readdir implementation keeps track of the offsets of the
* directory entries. It uses the offset parameter and always
* passes non-zero offset to the filler function. When the buffer
* is full (or an error happens) the filler function will return
* '1'.
*
* When FUSE_READDIR_PLUS is not set, only some parameters of the
* fill function (the fuse_fill_dir_t parameter) are actually used:
* The file type (which is part of stat::st_mode) is used. And if
* fuse_config::use_ino is set, the inode (stat::st_ino) is also
* used. The other fields are ignored when FUSE_READDIR_PLUS is not
* set.
*/
FUSE_DARWIN_EXTEND_OPERATION(
readdir,
int (*) (const char *, void *, fuse_fill_dir_t, off_t,
struct fuse_file_info *, enum fuse_readdir_flags),
int (*) (const char *, void *, fuse_darwin_fill_dir_t, off_t,
struct fuse_file_info *, enum fuse_readdir_flags)
)
However, the updated MacFuse uses the new FSKit backend instead of kernel extension backend. This will sometimes deliver a non-zero offset, and thus lead to abort and disconnection.
There was a problem hiding this comment.
But won't the assert cause the code to bail on a non-zero offset? Do you perhaps want to #ifdef the assert so it isn't active on macOS?
There was a problem hiding this comment.
I have replaced the assert(offset == 0) with if (offset !=0) return 0, which will return early instead of aborting the sshfs process. Although this may affect the FSKit backend behavior I mentioned, it works well for me so far. By the way, the #ifdef flag suggestion is a good point. I will update the PR shortly.
There was a problem hiding this comment.
Oh, I got what you meant in the beginning. There was a typo and have been fixed in the update =)
On macOS with macFUSE 5.3.x (FSKit backend) and macOS 27 Golden Gate beta, Finder/Spotlight may call readdir with a non-zero offset even when fuse_fill_dir is always called with offset=0 (old-style readdir). This triggers the assert and aborts the sshfs process. Replace assert(offset == 0) with if (offset != 0) return 0 in: - sftp_readdir_async() in sshfs.c - sftp_readdir_sync() in sshfs.c - cache_readdir() in cache.c Since all entries are returned in the first (offset=0) call, returning 0 on subsequent calls is semantically correct and prevents the crash. Fixes: libfuse#338 Tested: macOS 27 Golden Gate beta 2, macFUSE 5.3.2
|
Please see macfuse/macfuse#1131 (comment) for some (failing) test results. |
|
@dmik Thanks for testing and for the very clear repro — you were right, PR #371 only removed the assertion, it didn't fix the underlying problem. Root cause. sshfs uses old-style Fix. Instead of streaming from the single-pass handle, snapshot the entire directory into memory on the first Testing. I reproduced your scenario on a real remote mount (an HPC filesystem over a ProxyJump) and drove it hard with VS Code (large project + .git, global search, file watchers), with logging added to the cache layer. With the Branch: https://github.com/Yike-Ye/sshfs/tree/fix/readdir-full-snapshot @h4sh5 — happy to open a separate PR for this if you'd like to take it. Otherwise I'm fine leaving #371 as-is for now: macFUSE is actively reworking this area for macOS 27 support, so things may shift and I'd rather avoid conflicts until that settles. Just let me know which you prefer. |
|
@Yike-Ye thanks for the proper fix! I tested it with my stress test and it passes. The directory contents survives and everything else seems to function well. A few notes:
|
|
@dmik Thanks for testing and confirming it passes — great to have independent verification on the kernel backend. And you're right that the offset != 0 behaviour comes from the macOS kernel, so it affects both backends. Thanks especially for the correction on caching — you're right, it's not the cause. The real cause is On daemonization — agreed, and macFUSE 5.3.3 already moved that way on the libfuse side: it defers the actual mount until first session use and avoids forking in Framing for anyone following: #371 and the snapshot branch are two versions of the same readdir fix, not separate issues.
@h4sh5 two ways to land this, your call:
For clarity, this is entirely within sshfs's readdir callback ( |
|
When enumerating a directory, the FSKit API does not provide the number of directory entries it expects us to return (or a buffer size for us to fill). This means macFUSE does not know how many directory entries to request from libfuse (and by extension the file system server). This means the macFUSE FSKit backend may request more directory entries than FSKit can handle during a particular call. This results in the next readdir call re-requesting directory entries that have already been returned in the previous response. This might be related to the issue. |
|
@bfleischer Thanks, that's a really helpful explanation — it accounts for the non-zero Since this stems from the FSKit API itself, I'd imagine it won't need major changes on the macFUSE side, so the snapshot commit should be able to serve as a long-term fix here. Either way, we'll see how the sshfs maintainer wants to proceed. |
|
@Yike-Ye I have not had time to look into this more closely yet because I have been busy with macFUSE 5.3, but it looks like either macFUSE or sshfs is breaking the FUSE contract here. The fact that sshfs is affected could mean that other file systems are affected as well. If so, I may need to add a workaround in macFUSE. However, if sshfs is the one breaking the FUSE contract and the issue is isolated to sshfs, then a workaround on the macFUSE side may not be necessary. |
|
@Yike-Ye Some more details. It appears that something's still wrong with your latest fix. Under my stress test (VS Code running and open on a large .git project), the directory contents is reported by When I get back to stock sshfs-3.7.6 (i.e. without your patches at all) directory contents expectedly disappears, but I don't see this weird file size behavior. I can freely edit files in VS Code (files themselves are readable and writable, it's only the directory listing that gets empty) and their sizes remain consistent. PS. Forgot to mention that I somehow don't see offset == 0 assertions in stock sshfs-3.7.6 with macFUSE 5.3.3 at all. Maybe there are some related changes that affect that but still, it's strange. We are missing something. PPS. Using sshfs 2.10 with macFUSE 5.3.3 shows nor crashes neither missing directory contents. And no weird file size behavior when editing files in VS Code. So the problem of sshfs 3.x may be bigger than just this offset issue… I recalled now that I tried sshfs 3.x some months ago (before I switched to macFUSE 5.2 and even before switching to macOS 26 at all) and I got annoyed by disappearing directories so I went back to sshfs 2.10 back then (until the recent round). |
|
@dmik Thanks for the follow-up — that repro pinned it down. Both symptoms trace back to my first patch: it snapshotted the directory once per I've fixed this in 30bf21b by making the snapshot per-enumeration instead of per-opendir. On I tested it on localhost with a process holding a single directory handle open (to mimic the VS Code case): new files appear and file sizes update within the same open session, and concurrent listings stay complete and stable. Could you run it through your stress test? Branch: https://github.com/Yike-Ye/sshfs/tree/fix/readdir-full-snapshot Let me know if there is any issue on this fix. |
|
@dmik On macFUSE 5.3.3 I think the crash this PR guards against and the recent macFUSE 5.3.3 now refuses to fork inside In other words, the problem was never being in the background — it's forking a process that already holds the mount state. As long as the fork happens before To keep sshfs backgrounding as before, I reordered the macOS-only path (
This still daemonizes — it just forks before the mount, so those subsystems are never crossed by a fork. Same principle as 5.3.3 deferring the mount until after daemonization. The non-Apple path is left untouched to avoid any Linux regression. Confirmed working on macOS 27 + macFUSE 5.3.3: two mounts (one key-auth, one keyboard-interactive/password) both daemonize to the background as before, keep their SSH connections alive, and enumerate directories with no offset crash — stable across 2h+ of uptime. Change: Yike-Ye@b85d744 |
Regarding the
|
cache_readdir still had the pattern rejected in PR libfuse#371: an assert(offset == 0) on non-Apple platforms and an early "return 0" on macOS. As bfleischer pointed out, being called with a non-zero offset is valid under the FUSE contract on every platform: the two readdir modes are internal to libfuse, so the kernel or FSKit may legitimately start an enumeration at a saved cookie. A mode-1 filesystem must then still hand the complete listing to the filler with offset 0 (libfuse caches the entries and does the slicing); returning nothing is interpreted as an empty directory and makes entries silently vanish. Drop the assert and the early return, ignore the offset entirely, and always request a fresh full enumeration from the underlying filesystem when the cache is stale. This also removes the platform ifdef, since the correct behaviour is identical on Linux and macOS. Refs: libfuse#338 Refs: libfuse#371 (comment) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On macOS with macFUSE 5.3.x (FSKit backend) and macOS 27 Golden Gate beta, Finder/Spotlight may call readdir with a non-zero offset even when fuse_fill_dir is always called with offset=0 (old-style readdir). This triggers the assert and aborts the sshfs process.
Replace assert(offset == 0) with if (offset != 0) return 0 in:
Since all entries are returned in the first (offset=0) call, returning 0 on subsequent calls is semantically correct and prevents the crash.
Fixes: #338
Tested: macOS 27 Golden Gate beta 2, macFUSE 5.3.2