Skip to content

fix: replace assert(offset==0) with early return in readdir #371

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.

Sign up for GitHub

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

Open
Yike-Ye wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: replace assert(offset==0) with early return in readdir #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion cache.c
View file
Open in desktop

@dtgriscom dtgriscom Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why assert that offset must be 0, and then handle the case where it isn't?

@Yike-Ye Yike-Ye Jun 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@dtgriscom dtgriscom Jun 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Yike-Ye Yike-Ye Jun 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Yike-Ye Yike-Ye Jun 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I got what you meant in the beginning. There was a typo and have been fixed in the update =)

Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,13 @@ static int cache_readdir(const char *path, void *buf, fuse_fill_dir_t filler,
struct node *node;
struct cache_dirent **cdent;

assert(offset == 0);
#ifndef __APPLE__
assert(offset == 0); /* keep for non-macOS; see sshfs.c sftp_readdir_async */
#endif
/* On macOS with macFUSE 5.3.x / FSKit, may be called with offset != 0.
* All entries were already returned in the first (offset=0) call. */
if (offset != 0)
return 0;

pthread_mutex_lock(&cache.lock);
node = cache_lookup(path);
Expand Down
22 changes: 21 additions & 1 deletion sshfs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2318,7 +2318,21 @@ static int sftp_readdir_async(struct conn *conn, struct buffer *handle,

int done = 0;

assert(offset == 0);
/*
* sshfs uses old-style readdir (filler always called with offset=0),
* so FUSE should return all entries in one call (offset=0).
* On macOS with macFUSE 5.3.x / FSKit backend, Finder or Spotlight
* may call readdir again with a non-zero offset even in old-style mode.
* When that happens, all entries were already returned in the first
* call, so we can safely return 0 (no more entries) instead of crashing.
* See: https://github.com/libfuse/sshfs/issues/338
*/
#ifndef __APPLE__
assert(offset == 0);
#endif
if (offset != 0)
return 0;

while (!done || outstanding) {
struct request *req;
struct buffer name;
Expand Down Expand Up @@ -2390,7 +2404,13 @@ static int sftp_readdir_sync(struct conn *conn, struct buffer *handle,
void *buf, off_t offset, fuse_fill_dir_t filler)
{
int err;
/* See comment in sftp_readdir_async for why offset != 0 is handled
* this way instead of asserting. */
#ifndef __APPLE__
assert(offset == 0);
#endif
if (offset != 0)
return 0;
do {
struct buffer name;
err = sftp_request(conn, SSH_FXP_READDIR, handle, SSH_FXP_NAME, &name);
Expand Down