fix: handle readdir offset != 0 by snapshotting the directory#379
Open
Yike-Ye wants to merge 4 commits into
Open
fix: handle readdir offset != 0 by snapshotting the directory#379Yike-Ye wants to merge 4 commits into
Yike-Ye wants to merge 4 commits into
Conversation
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
The previous fix (66b3724) replaced assert(offset==0) with `return 0`. That stopped the crash but not the root cause, as dmik reported in macfuse/macfuse#1131: under heavy concurrent access the directory contents still vanish. sshfs uses old-style readdir over a single-pass SFTP directory handle. When macFUSE 5.3.x's FSKit backend re-issues readdir on the same handle (non-zero offset, or an offset==0 rewind), the already-exhausted handle yields nothing, so readdir returns an empty listing. The directory cache (cache.c, 20s) then caches that empty result, so the directory appears empty for ~20s until the cache expires -- exactly the "contents vanish then reappear" behaviour that was reported. Fix: snapshot the entire directory into memory at the first readdir and serve every subsequent readdir call (any offset, including rewinds) from that snapshot. This makes readdir idempotent -- the kernel can re-issue it with any offset and always gets the complete listing -- so the cache never stores a partial/empty result. Tested on a real remote mount (HPC over ProxyJump) under heavy VS Code load, with logging added to the cache layer: readdir returns the full listing every time, zero empty results, directory no longer disappears. Refs: libfuse#338 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous commit snapshotted the directory once per opendir and served every readdir from that snapshot. That stopped the vanishing listings, but froze the snapshot for the whole opendir session: when an application holds a directory open and re-reads it (e.g. VS Code watching a project), newly created files never appeared and the cached stat sizes went stale -- dmik reported files looking truncated as the stale size served by readdir fought the live size from getattr. Make the snapshot per-enumeration instead of per-opendir: on offset == 0 (a fresh listing or a rewinddir) reopen a fresh SFTP handle and re-read the directory from the server, so newly created files and updated attributes show up; offset != 0 continues to be served from the snapshot taken at offset 0 so the listing never comes back empty on an already-exhausted handle. The directory path is stored in the handle so reopening works even when readdir is called with a null path. Verified on localhost with a process holding a single directory handle open: newly created files appear and file sizes update within the same open session, while concurrent listings stay complete and stable. Refs: libfuse#338 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces #371 (closed).
Problem
sshfs uses mode-1 readdir — it ignores the offset and hands the whole listing to the filler with offset 0 — but also asserted
offset == 0in three places:sftp_readdir_async()(sshfs.c)sftp_readdir_sync()(sshfs.c)cache_readdir()(cache.c)That assertion is an invalid assumption, and a long-standing one. Calling readdir with
offset != 0is legal, and the mismatch was already reported in #211 (2020, on FreeBSD viaseekdir) — where @Nikratio confirmed the code "is buggy ... it certainly can be fixed" and suggested exactly this fix: "always retrieve the full list from the SFTP server and buffer it in memory." A mode-1 filesystem must still return the full listing when called with a non-zero offset, rather than assert or return nothing.The bug only became acute on macOS with macFUSE 5.3.x. macFUSE is transitioning its backend from the kernel extension toward Apple's FSKit framework (as macOS deprecates third-party kexts), and as part of that transition 5.3.x reworked directory enumeration — 5.3.1 switched to the node identifier and generation as the authoritative handle instead of the inode number, and added a
FUSE_LOOKUPon the entries returned byFUSE_READDIR. We can only speculate that this is whyreaddiris now called withoffset != 0so much more often (#338), but the fix does not depend on the cause. It is sshfs's own contract violation — the process aborts on the assert, and the earlierreturn 0attempt (#371) empties the listing instead — and it is observed on macFUSE 5.3.x on both the FSKit and kernel-extension backends.Fix
Remove the
assert(offset == 0)in all three locations (and drop the now-unused#include <assert.h>from cache.c), and handle a non-zero offset correctly by buffering the whole directory, along the lines @Nikratio suggested:sshfs_readdir()keeps a snapshot of the current enumeration. An SFTP directory handle is single-pass — once read toSSH_FX_EOFit yields nothing — so onoffset == 0(a fresh listing or a rewinddir) it re-opens the handle if it has been exhausted and re-reads the whole directory from the server, rebuilding the snapshot. A continuation (offset != 0) is served from that snapshot, always filling with offset 0 so libfuse does the slicing.offset == 0rather than once per open handle.cache_readdir()ignores the offset entirely and, when the cache is stale, always requests a fresh, complete enumeration. (@dmik confirmed the vanishing itself was not caused by caching — it reproduced with the cache disabled — so this is for contract-correctness, not the primary fix.)No platform
#ifdefis involved: handlingoffset != 0this way is correct on every platform, so the assertions are dropped unconditionally.Testing
@dmik stress-tested this branch (VS Code on a large
.gitproject, overnight idle with-o ServerAliveInterval=15 -o ServerAliveCountMax=3), including the newly-created-files case, and reports stable directory listings and file editing (#338).Refs: #338, #371, #211