Correctly set FileOffsets in WriteHandle#14562
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes wsl::windows::common::relay::WriteHandle so that when the underlying handle is a seekable file, overlapped writes advance the file offset instead of repeatedly writing at the same position (which could overwrite/truncate previously written data).
Changes:
- Add per-handle offset tracking to
WriteHandleand apply it toOVERLAPPED.Offset/OffsetHighfor each write. - Advance the tracked offset on both synchronous and asynchronous write completion paths.
- Add a new Windows test that validates
WriteHandlebehavior for pipes and disk files.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/windows/common/relay.hpp |
Adds stored offset state to WriteHandle. |
src/windows/common/relay.cpp |
Initializes and updates WriteHandle’s file offset and sets OVERLAPPED offsets before each write. |
test/windows/WSLCTests.cpp |
Adds WriteHandleContent test covering pipe and file output scenarios. |
| wsl::windows::common::relay::MultiHandleWait io; | ||
|
|
||
| io.AddHandle(std::make_unique<wsl::windows::common::relay::ReadHandle>( | ||
| std::move(readPipe), [&](const gsl::span<char>& buffer) { readData.append(buffer.data(), buffer.size()); })); |
There was a problem hiding this comment.
The ReadHandle callback appends buffer.data() even when buffer can be an empty span (e.g., EOF/broken pipe paths may invoke the callback with an empty span whose .data() is null). Passing a null pointer to std::string::append(ptr, 0) is undefined behavior and can crash under some CRT/debug configurations. Guard on buffer.empty() (or append only when buffer.size() > 0).
| std::move(readPipe), [&](const gsl::span<char>& buffer) { readData.append(buffer.data(), buffer.size()); })); | |
| std::move(readPipe), | |
| [&](const gsl::span<char>& buffer) | |
| { | |
| if (!buffer.empty()) | |
| { | |
| readData.append(buffer.data(), buffer.size()); | |
| } | |
| })); |
| auto outputFile = wil::create_new_file(L"write-handle-test", GENERIC_WRITE | GENERIC_READ, 0, nullptr); | ||
|
|
||
| auto cleanup = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() { | ||
| outputFile.reset(); | ||
| std::filesystem::remove("write-handle-test"); |
There was a problem hiding this comment.
This test creates a fixed relative filename ("write-handle-test") in the current working directory and uses CREATE_NEW semantics. If a previous run left the file behind (crash/abort) or tests run concurrently in the same working dir, this can become flaky. Prefer creating the file under std::filesystem::temp_directory_path() with a unique name (and/or do a best-effort pre-cleanup) and remove via the same full path.
| auto outputFile = wil::create_new_file(L"write-handle-test", GENERIC_WRITE | GENERIC_READ, 0, nullptr); | |
| auto cleanup = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() { | |
| outputFile.reset(); | |
| std::filesystem::remove("write-handle-test"); | |
| std::filesystem::path outputFilePath = | |
| std::filesystem::temp_directory_path() / | |
| (L"write-handle-test-" + std::to_wstring(GetCurrentProcessId()) + L".tmp"); | |
| auto outputFile = wil::create_new_file(outputFilePath.c_str(), GENERIC_WRITE | GENERIC_READ, 0, nullptr); | |
| auto cleanup = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&]() { | |
| outputFile.reset(); | |
| std::error_code ec; | |
| std::filesystem::remove(outputFilePath, ec); |
Summary of the Pull Request
This change updates WriteHandle to correctly set offsets when writing to its handle.
If the handle was a file, then WriteHandle would always write to the same offset, effectively incorrectly truncating the file
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed