Fix positioned I/O semantics on Windows and guard read/write sizes - #350
Fix positioned I/O semantics on Windows and guard read/write sizes#350YayoRazo wants to merge 1 commit into
Conversation
Two related robustness fixes in the Windows syscall adapters. pread/pwrite: the adapters issue an OVERLAPPED ReadFile/WriteFile at the requested offset, but on a synchronous handle that call also advances the underlying file position. POSIX pread/pwrite must leave the offset untouched, so interleaving positioned and sequential I/O silently read or wrote the wrong bytes. Save the current position before the call and restore it in a defer, mirroring what ftruncate already does. Added a cross-platform regression test. read/write: unlike pread/pwrite, these passed the byte count straight to the CRT via numericCast with no bounds check. A count exceeding the unsigned int that _read/_write accept would trap. Add the same `nbyte > DWORD.max` guard the positioned variants use so the error surfaces as EINVAL instead, and a Windows regression test that drives an oversized count through read and write.
| Int(_read(fd, buf, numericCast(nbyte))) | ||
| // _read takes an unsigned int count; reject sizes that would overflow it so | ||
| // numericCast cannot trap (pread/pwrite apply the same guard). | ||
| if nbyte > Int(DWORD.max) { |
There was a problem hiding this comment.
Is there a way to get a negative value of nbyte to this function from a public entry point, using safe code?
There was a problem hiding this comment.
Even allowing for unsafe code, this replaces a trap in numericCast(nbyte) with a thrown error. Is this an improvement? We can note that in this situation the POSIX version effectively throws an Errno(EINVAL).
There was a problem hiding this comment.
According to https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read?view=msvc-170, the expectation is that _read sets errno to EINVAL for such a case. We should take the nbyte guard, then.
There was a problem hiding this comment.
guard let count = UInt32(exactly: nbyte) else { ... }
|
Could you separate this in two PRs, one for each of the items in the summary above? We much prefer to have one change per commit; it makes the history much more explicable. |
2 participants
Summary
Two related robustness fixes in the Windows syscall adapters.
pread / pwrite must not move the file offset
The adapters issue an
OVERLAPPEDReadFile/WriteFileat the requested offset, but on a synchronous handle that call also advances the underlying file position. POSIXpread/pwritemust leave the offset untouched, so interleaving positioned and sequential I/O silently read or wrote the wrong bytes (areadafter apreadresumed from the wrong place).Fix: save the current position before the call and restore it in a
defer, mirroring whatftruncatealready does in the same file.read / write must guard oversized counts
Unlike
pread/pwrite(which already validatenbyte > DWORD.max), the sequentialread/writeadapters passed the byte count straight to the CRT vianumericCastwith no bounds check. A count exceeding the unsignedintthat_read/_writeaccept would trap. Add the samenbyte > DWORD.maxguard so the error surfaces asEINVAL.Testing
testPositionedIODoesNotMoveFileOffset(cross-platform): parks the file offset, performs a positioned read and write, and asserts the offset is unchanged. Verified it fails without the fix (the offset moves) and passes with it. Because POSIXpread/pwritenever move the offset, this also guards the behavior on Linux/Darwin.testSequentialBufferSizeLimit(Windows): drives an oversized count (DWORD.max + 1) over a small allocation throughreadandwriteand assertsEINVAL. Verified it traps without the fix and passes with it.swift testsuite green on Windows (x86_64-unknown-windows-msvc, Swift 6.3.3); no behavioral change on other platforms.