Create and write files whose mode carries no owner-write bit - #65
Open
Ebrathul wants to merge 3 commits into
Open
Create and write files whose mode carries no owner-write bit#65Ebrathul wants to merge 3 commits into
Ebrathul wants to merge 3 commits into
Conversation
openVFSfuse_write() discarded fuse_file_info and re-opened the backing file with O_WRONLY on every call. openVFSfuse_read() already reads from fi->fh and openVFSfuse_release() closes it, so write() was the only operation not using the descriptor open() had stored. Two consequences. An open()/close() pair ran per write call. And the mode bits were re-evaluated on every write, so a descriptor stayed writable only as long as the file's mode allowed a fresh open: writing through an fd that was opened before the file was made read-only failed with EACCES, where POSIX requires it to succeed. Assisted-by: Claude Opus 5 (Anthropic)
Without a create operation libfuse falls back to mknod() plus a separate open(). openVFSfuse_mknod() creates the file with the mode the caller asked for -- applied literally, because main() sets umask(0) -- and closes it. The open() that follows is then a real permission check against a file that is already read-only, and fails with EACCES, leaving a zero-length file behind. Creating a file with a mode that has no owner-write bit and writing to the returned descriptor is ordinary: git creates every loose object with mkstemp mode 0444, and cp -p reproduces the source mode. On a local filesystem one open() call does both halves and the mode is never consulted for the descriptor it returns. Doing the same here means implementing create(), which creates and opens in one step and hands back the descriptor. mknod() is left alone: after this change nothing routes an O_CREAT open through it, and mknod(0444) followed by open(O_WRONLY) failing is correct. Assisted-by: Claude Opus 5 (Anthropic)
Report ownership failures from create and remove the incomplete file instead of returning a descriptor with unexpected ownership. Preserve syscall errors across debug logging and pass correctly typed values to the variadic logger. Assisted-by: Codex:GPT-5
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.
I have been running openvfs on my own account with my own tool and the desktop-client plug-in
from nextcloud/desktop#10635, to find out what still stands between it and
everyday use of a sync root. The first thing that stopped me was files without
an owner-write bit: I could not create one inside the mount, which among other
things means a git repository cannot be written to there.
Creating a file with a read-only mode and writing to the descriptor that created
it fails with
EACCESon an openvfs mount. Two independent defects produce theone symptom, and neither fixes it alone.
There is no
create()operation, so libfuse falls back tomknod()plus aseparate
open().openVFSfuse_mknod()creates the file with the mode thecaller asked for — applied literally, because
main()callsumask(0)— andcloses it:
The
open()that follows is then a real permission check against a file that isalready read-only. It fails, and the zero-length file stays behind. On a local
filesystem one
open()does both halves, and the mode it is given is neverconsulted for the descriptor it returns.
openVFSfuse_write()discards the descriptor and re-opens the backing fileon every call:
openVFSfuse_read()already reads fromfi->fhandopenVFSfuse_release()closes it, so
write()was the only operation not using whatopen()stored.Besides an
open()/close()pair per write call, this re-evaluates the mode onevery write: a descriptor stays writable only as long as a fresh
open()wouldsucceed.
Creating a file read-only and writing through the returned descriptor is
ordinary, not application-specific: git creates every loose object with
git_mkstemp_mode(..., 0444),cp -preproduces a read-only source mode, andtar -xrestores read-only modes from an archive. Writing to a descriptor afterthe file's mode changed is plain POSIX — the mode is checked at
open()and notagain.
Reproduction
Creating a file and writing one byte to the returned descriptor, per mode. Same
script on tmpfs for comparison:
The owner-write bit is the discriminator. Each failure leaves a zero-length file
behind, which is
mknod()having already succeeded.The write half reproduces without creating anything:
In git terms, on a mount inside a sync root:
Reading a repository works —
status,log, and a tree copied in withcp -aare all fine. Only writing fails.
The two defects are independent. With
create()implemented butwrite()leftalone, creation succeeds and the first write fails instead:
Hence one pull request with two commits rather than two pull requests.
The change
create()creates and opens in one step and hands the descriptor back, the waylibfuse's own
passthrough.cdoes —open(path, fi->flags, mode), flags passedthrough untouched. The kernel sets
O_CREATitself on a create request, so thereis nothing to add to them; on a mount instrumented to print what arrives:
O_EXCLarrives too and is honoured by passing the flags through, which keepsthe race behaviour a caller asked for.
write()readsfi->fhdirectly rather than hedging on a path-based open,because
read()andrelease()already assume the same invariant.The error paths save
errnobefore debug logging, since formatting and syslogmust not change the error returned to FUSE.
create()also assigns ownershipthrough the open descriptor. If that fails, it removes the incomplete file,
closes the descriptor, reports any cleanup failure, and returns the original
ownership error instead of leaving a file with unexpected ownership behind.
The variadic log arguments use types matching their format specifiers.
mknod()is deliberately left alone. After this the ordinary create path nolonger routes through it, and
mknod(0444)followed byopen(O_WRONLY)failingis correct behaviour that should keep working.
A file that does not exist yet cannot be a placeholder, so
create()needsneither hydration nor placeholder attributes —
getxattr()already synthesises ahydrated default for a file that carries none.
No test. This bug lives in the FUSE layer, and
mainhas no harness thatmounts a filesystem —
ctestbuildsappstreamtestonly. #64 introducessocketthreadtest, but on the other side of that boundary, and adding a secondparallel harness alongside it seemed wrong. Happy to turn the scripts above into
a test in whatever shape you would like once #64 has settled.
openVFSfuse_truncate()ignoresfithe same waywrite()did and fails thesame way. Separate defect, separate fix, separate pull request.
Relationship to the open pull requests
Independent of #61–#64. #64 rewrites the hydration wait inside
openVFSfuse_open()and #61 rewrites the socket read path; neither toucheswrite(),mknod(), the operations table, nor adds acreate(). No ref in thisrepository implements
create()today.These commits apply cleanly to
mainatcbdeeef, and I also cherry-picked themonto #64's head and built there:
ctestgreen including #64's newsocketthreadtest, and the acceptance list below re-run on that combination withthe same results. Tested, not assumed — whichever of these lands first, the other
still applies.
Environment
Linux 7.1.8 (CachyOS), libfuse 3.18.2, Btrfs.
Applied to a fresh clone of
mainatcbdeeefand built there. BuiltRelWithDebInfowith gcc 16.2.1, gcc 15 and clang 22 — clean on all three,ctestgreen, clang-format clean. Under-Wall -Wextra -Wconversion -Wsign-conversionthe added lines warn exactly as the existingfi->fh = resinopen(),pread(fi->fh, ...)inread()andclose(fi->fh)inrelease()do,and no other way.
Exercised against a real sync root:
git init,git addand two commits over250 files succeed,
git fsckclean, objects created0444,cp -pof aread-only file and
tar -xpreserving read-only modes both work,O_APPENDwrites and interleaved
pwriteoffsets stay correct, a 60 MB write isbyte-identical locally and after upload, and a dehydrated file still hydrates on
read to a checksum identical to the server's.