Repository navigation
Create the destination exclusively in writeToFile - #1183
xianshijing-lk wants to merge 2 commits into
Conversation
`writeToFile` already refuses a sender-chosen name that is not a plain file component, so the write stays inside the directory the caller chose. It then opened the destination through `AsyncFileStream(writingTo:)`, whose mode is `O_WRONLY | O_TRUNC | O_CREAT`. That truncates a file already at the path, and follows a symlink at the final component to wherever it points -- including outside the directory. The name comes from the stream header, so a sender can aim at a file the application already has in its chosen directory. Planting the symlink needs local access, so the truncation case is the remotely reachable half; both are closed the same way. Open with `O_WRONLY | O_CREAT | O_EXCL` instead. The existence check and the open become one syscall, so there is no check-then-open window, and POSIX requires `O_CREAT | O_EXCL` to fail on a symlink at the final component rather than follow it. `writingTo` is renamed `creatingNewAt` so the call sites say what they do; it had one non-test caller. `EEXIST` surfaces as `StreamError.io(reason:)`. A dedicated case would let callers branch on it, but `StreamError` is a public non-frozen enum, so adding one breaks exhaustive switches in consumer code and means touching the pinned `errorCode` table and the FFI mapping. `.io` already means "reading or writing a file failed", so it carries this without the churn. This is an intentional behaviour change: an existing destination now throws instead of being replaced. Callers that want replacement must choose that policy themselves. `readToFile` now writes into a fresh directory. It wrote a fixed name into the shared temporary directory and relied on replacing whatever a previous run left there, which exclusive creation no longer allows. Three regression tests, each failing before the change: the reader does not overwrite an existing file, does not write through a symlink to its target, and `AsyncFileStream` reports `EEXIST` for an occupied path.
|
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
pblazej
left a comment
pblazej
left a comment
There was a problem hiding this comment.
The exclusive create is correct, and the new tests catch a regression. Two small items block: the Lint failure and the undocumented behavior change.
| /// instead of replacing it. The existence check and the open are one syscall, so there is no | ||
| /// window between them, and POSIX requires `O_CREAT | O_EXCL` to fail on a symlink at the | ||
| /// final component rather than following it to its target. | ||
| private static let mode = O_WRONLY | O_CREAT | O_EXCL |
There was a problem hiding this comment.
praise: One O_CREAT | O_EXCL open closes both the overwrite case and the symlink case.
| #expect(readBuffer == testBuffer) | ||
| } | ||
|
|
||
| @Test func writeRefusesAnExistingPath() async throws { |
There was a problem hiding this comment.
issue (blocking): This test never awaits, so SwiftFormat redundantAsync fails Lint. Remove async.
There was a problem hiding this comment.
Fixed. Reproduced it first to be sure it was the same rule:
AsyncFileStreamTests.swift:68:1: error: (redundantAsync) Remove redundant async keyword from function declarations that don't contain any await expressions.
async dropped. Repo-wide swiftformat --lint is clean on everything this PR touches.
One unrelated thing it surfaced: Tests/LiveKitCoreTests/Broadcast/BroadcastAudioCodecTests.swift:102 fails the indent rule. I checked it against clean origin/main and it fails there too, so it is pre-existing and not from this PR — but Lint will stay red until someone fixes it.
| let writer: AsyncFileStream<WriteMode> | ||
| do { | ||
| writer = try AsyncFileStream(creatingNewAt: fileURL) | ||
| } catch AsyncFileStream<WriteMode>.Error.openFailed(EEXIST) { |
There was a problem hiding this comment.
issue (blocking): writeToFile now throws StreamError.io for an existing file and no longer overwrites it. Document this in the docstring and add a .changes entry.
There was a problem hiding this comment.
Both done.
The docstring now states the destination is created exclusively — an existing file makes the write fail and is left untouched rather than replaced, a symlink at the path is not followed — and lists StreamError.io(reason:) as thrown for an occupied destination, alongside a pointer to nameOverride for callers that need a specific path.
The .changes entry is type="changed" rather than "fixed", since callers relying on the overwrite have to pick an explicit destination or remove the file first.
| try Data("original".utf8).write(to: victim) | ||
|
|
||
| let (reader, manager) = try await openReader() | ||
| await #expect(throws: StreamError.self) { |
There was a problem hiding this comment.
suggestion (non-blocking,test): Match .io here and at line 130, not only StreamError.
There was a problem hiding this comment.
Done, via an expectIOError helper used at both sites.
It matches the case rather than a whole value: io carries the open's description, so pinning the message would couple the tests to the error text. The helper forwards sourceLocation, so a failure still reports against the calling test rather than the helper.
Drop `async` from `writeRefusesAnExistingPath`. It never awaits, so SwiftFormat's `redundantAsync` rule failed Lint. Reproduced the failure before and after: AsyncFileStreamTests.swift:68:1: error: (redundantAsync) Remove redundant async keyword from function declarations that don't contain any await expressions. Document the behavior change on `writeToFile` and add a `.changes` entry. The docstring now states that the destination is created exclusively: an existing file makes the write fail and is left untouched rather than replaced, a symlink at the path is not followed, and `StreamError.io` is listed as thrown for an occupied destination. The changeset is `type="changed"`, not `"fixed"`, since callers that relied on the overwrite have to pick an explicit destination now. Match `StreamError.io` in the two reader tests rather than any `StreamError`, via an `expectIOError` helper. It matches the case and not a whole value, because `io` carries the open's description and pinning that text would couple the tests to the message. The helper forwards `sourceLocation` so a failure still points at the call site.
|
Pushed b9cf402 — both blockers plus the non-blocking suggestion. Lint — Documented behaviour change — the Tighter assertions — both reader tests now match All 22 tests pass, and I re-confirmed all three still fail with the fix reverted.
|
pblazej
left a comment
pblazej
left a comment
There was a problem hiding this comment.
Both blockers are fixed, and the .io tests fail when the error mapping changes. The remaining comments are non-blocking.
| /// ``StreamError`` if reading the stream fails. `CancellationError` at once if the calling task | ||
| /// is cancelled; the file then holds what was written so far. | ||
| /// the stream information, so this check keeps the file inside `directory`. | ||
| /// ``StreamError/io(reason:)`` if something already exists at the destination, or if the |
There was a problem hiding this comment.
suggestion (non-blocking): Only EEXIST maps to .io, so a missing parent directory still throws the internal openFailed(ENOENT). Narrow this line to an occupied destination.
| /// - directory: The directory to write the file in. The system temporary directory is used if not specified. | ||
| /// - nameOverride: The name to use for the written file. If not specified, file name and extension will be automatically | ||
| /// inferred from the stream information. | ||
| /// The destination is created exclusively: if anything already exists at that path the write |
There was a problem hiding this comment.
nitpick: Say that a failed or cancelled transfer leaves a partial file, so a retry with the same name throws .io.
| _ = manager | ||
| } | ||
|
|
||
| /// Asserts `body` fails with ``StreamError/io(reason:)`` specifically. |
There was a problem hiding this comment.
nitpick: #expect { … } throws: { if case .io = $0 as? StreamError { true } else { false } } can replace this helper.
Swift counterpart of livekit/rust-sdks#1605.
What
writeToFilealready refuses a sender-chosen name that is not a plain file component, so the write stays inside the directory the caller chose — that part is in good shape onmain. It then opened the destination throughAsyncFileStream(writingTo:), whose mode isO_WRONLY | O_TRUNC | O_CREAT. That truncates a file already at the path, and follows a symlink at the final component to wherever it points, including outside the directory.The name comes from the stream header, so a sender can aim at a file the application already has in its chosen directory. Planting the symlink needs local access, so the truncation case is the remotely reachable half; both are closed the same way.
How
Open with
O_WRONLY | O_CREAT | O_EXCL. The existence check and the open become one syscall, so there is no check-then-open window, and POSIX requiresO_CREAT | O_EXCLto fail on a symlink at the final component rather than follow it.writingTois renamedcreatingNewAtso call sites say what they do — it had one non-test caller.Error surface — reviewer decision welcome
EEXISTsurfaces asStreamError.io(reason:). A dedicated case would let callers branch on it programmatically, butStreamErroris a public non-frozen enum: adding one breaks exhaustive switches in consumer code and means touching the pinnederrorCodetable and the FFI mapping..ioalready means "reading or writing a file failed", so it carries this without the churn. Happy to add a case instead if you'd rather have it typed.Behaviour change
Intentional: an existing destination now throws instead of being replaced. Callers wanting replacement must choose that policy themselves.
readToFilenow writes into a fresh directory — it wrote a fixed name into the shared temporary directory and relied on replacing whatever a previous run left there, which exclusive creation no longer allows. Worth noting as evidence the old behaviour was being leaned on silently.Tests
Three regression tests, each verified to fail before the change and pass after:
writeToFileDoesNotOverwriteExistingFilewriteToFileDoesNotFollowSymlinkAsyncFileStreamTests.writeRefusesAnExistingPathAll DataStream suites plus
AsyncFileStreamTests→ 80 tests in 7 suites pass. Build andswiftlintclean.Scope
The caller still chooses the directory; this does not defend against an attacker who controls that directory itself.
🤖 Generated with Claude Code