fix: skip chown on filesystems without Unix ownership#326
Merged
Conversation
Pasting to a USB drive mounted as FAT/exFAT/NTFS surfaced "operation not permitted" from the post-copy chown even though the data was already on disk. These filesystems carry no on-disk owner/group fields and the kernel rejects chown(2) regardless of caller privilege; the rsync itself ran with --no-o --no-g so the copy was actually successful. Detect such mounts by statfs magic number and let Chown / ChownRecursive short-circuit with a warning, so paste / upload / download to External no longer fails on FAT-class drives. Other filesystems and afero-backed callers behave identically to before. Co-authored-by: Cursor <cursoragent@cursor.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.
Summary
operation not permittedon the post-copychown(2), even though the data is already written to the device.chown(2)regardless of caller privilege.statfsmagic number and short-circuitfiles.Chown/files.ChownRecursivewith a warning. Behaviour on every other filesystem, and on everyafero.Fs-backed caller, is byte-for-byte unchanged.Reproduction
/data/External/<id>.+ "```" +chown /data/External// to 1000:1000: chown ...: operation not permitted
+ "```" +Root cause
+ "pkg/tasks/task_paste_posix.go::rsync()" +runs+ "rsync --no-o --no-g" +(so rsync itself avoids preserving ownership) and then calls+ "files.Chown(nil, dstPath, 1000, 1000)" +to enforce+ "1000:1000" +on the result. FAT/exFAT/NTFS carry no on-disk ownership, so+ "chown(2)" +always returns+ "EPERM" +there. The error is propagated and the task is marked failed despite the data being fine.Fix
Add
+ "files.SupportsOwnership(path string) bool" +(Linux only,+ "syscall.Statfs" +-based). It returns+ "false" +only for these six filesystem magics, which cannot honour+ "chown" +regardless of caller privilege:+ "0x4d44" ++ "0x2011BAB0" ++ "0x5346544e" ++ "0x7366746e" ++ "0x9660" ++ "0x15013346" ++ "files.Chown" +and+ "files.ChownRecursive" +short-circuit with a warning when+ "SupportsOwnership" +returns+ "false" +. The guard inside+ "Chown" +is gated on+ "fs == nil" +, so every+ "afero.Fs" +-backed caller is left untouched.Impact
Unchanged on every non-FAT-class filesystem.
+ "SupportsOwnership" +falls back to+ "true" +on anything outside the table above and on+ "statfs" +errors, so the original code path runs verbatim on ext4 / btrfs / xfs / tmpfs / overlay / nfs / sshfs, etc.Unchanged for
+ "afero.Fs" +-backed callers:+ "pkg/drivers/posix/posix/posix.go" +,+ "pkg/preview/preview.go" +,+ "pkg/diskcache/file_cache.go" +, and the non-recursive branch of+ "pkg/hertz/biz/handler/api/permission/permission_service.go" +.Fixed in passing (same root cause): paste / upload / download /
+ "MkdirAllWithChown" +to External when the target is FAT/exFAT/NTFS - covers+ "task_paste_posix.go" +,+ "task_paste_sync.go" +,+ "task_paste_cloud.go" +,+ "task_paste_download.go" +, and+ "pkg/drivers/posix/upload/handlefunc.go" +.One behavioural shift to note: the recursive branch of the permission API on a FAT-class destination now returns
+ "200 OK" +(with a warning log) instead of+ "500" +. The Olares UI does not expose ownership for External, so the surface contract is preserved.Known limitations
FUSE-mounted ntfs-3g / exfat-fuse are not covered:
+ "statfs" +reports the FUSE magic, which is deliberately left out of the list. Modern Ubuntu (22.04+) ships native+ "ntfs3" +/+ "exfat" +drivers, so this is unlikely in practice. If it ever surfaces, a+ "/proc/self/mountinfo" +parser can be bolted on as a follow-up.Test plan
+ "gofmt" +clean+ "GOOS=linux GOARCH=amd64 go vet ./pkg/files/..." ++ "GOOS=linux GOARCH=arm64 go vet ./pkg/files/..." ++ "GOOS=linux GOARCH={amd64,arm64} go build ./pkg/files/..." ++ "go test -c" +compiles on both archs+ "ChownRecursive" +) to the same USB -> task succeeds+ "PUT /api/v1/permission" +on a Drive-side path (ext4) -> owner actually changes, behaviour identicalMade with Cursor