windows: delegate Read, Write, Fd, Name to the file passed to newMaster#88
windows: delegate Read, Write, Fd, Name to the file passed to newMaster#88vagokuln-msft wants to merge 2 commits into
Conversation
Previously, the Windows master struct hardcoded os.Stdin for Read/Fd and os.Stdout for Write, ignoring the file passed to ConsoleFromFile. This meant callers that created a console from os.Stderr (or any other valid file) would silently operate on the wrong stream. Store the file in the master struct and delegate Read, Write, Fd, and Name to it, matching the Unix implementation. Fixes containerd#83 Signed-off-by: Varun Gokulnath <vagokuln@microsoft.com>
515d88e to
820dfc5
Compare
|
@estesp @dmcgowan @thaJeztah do you have any thoughts on this PR? |
cpuguy83
left a comment
There was a problem hiding this comment.
This looks correct at least.
|
Ping, @estesp @dmcgowan @thaJeztah |
|
@copilot Help tag the maintainers to get a sign-off for this PR. It has been sitting for a while |
cpuguy83
left a comment
There was a problem hiding this comment.
Review from Claude Opus 4.8
Delegating Read/Write/Fd/Name to the stored file is the right fix and matches the Unix implementation. A few things before merging:
-
The reported bug isn't tested. #83 is about
Writehitting the wrong stream, but both tests only assertFd()andName(). Add an in-process test that constructs a master from a pipe/buffer-backed file and assertsWritegoes to that file, notos.Stdout. Unlike the E2E test, it would run in CI. -
TestNewMaster_FileDelegationis near-tautological.Fd()returnsm.f.Fd(), som.Fd() == tt.file.Fd()is mostlyf.Fd() == f.Fd(). It catches a revert touintptr(m.in)but covers neither the publicConsoleFromFilepath nor the I/O methods. -
The E2E test won't run in CI and hides failures.
OpenFile("CONIN$"/"CONOUT$")fails headless, so it skips. On failure the subprocess writes toos.Stderr, wired toconout, so the parent only reportsexit status 1with no detail. If kept, capture the subprocess output (CombinedOutput/buffer) intot.Fatalf. -
Incomplete delegation. Only the I/O methods follow
f;initStdios(),SetRaw(),Reset(),Size(), andDisableEcho()still operate on globalos.Std{in,out,err}. A console fromos.StderrreportsFd() == stderrand writes to stderr, butSize()queries stdout andSetRaw()flips all three. Either extend the fix to trackfor document the limitation. -
Name()behavior change. Now returns/dev/stdoutetc. instead of"console", and the explanatory comment was dropped. Aligns with Unix, but call it out in the description.
- Add in-process pipe-based test verifying master delegates Write and Read to its file (regression test for containerd#83 where output leaked to stdout). - Replace the tautological Fd/Name test with meaningful pipe-based assertions that also verify Fd() is not os.Stdout's descriptor. - Capture the E2E subprocess's stderr so failures are reported with detail. - Document on newMaster that console-mode operations act on the process's shared console object, not f alone. - Restore an explanatory comment on Name() noting it now returns the underlying file name. Signed-off-by: Varun Gokulnath <vagokuln@microsoft.com>
|
Thanks for the review! All five points are addressed in the latest commit:
|
|
Friendly ping for a review when you get a chance 🙏 — @AkihiroSuda @fuweid This is a small Windows-only fix for #83 (output written to a console created from a non-stdout stream leaked to stdout). CI is green, and |
Previously, the Windows master struct hardcoded os.Stdin for Read/Fd and os.Stdout for Write, ignoring the file passed to ConsoleFromFile. This meant callers that created a console from os.Stderr (or any other valid file) would silently operate on the wrong stream.
This PR stores the file in the master struct and delegates Read, Write, Fd, and Name to it, matching the Unix implementation.
Changes:
f Filefield to the master structRead()delegates tom.f.Read()instead ofos.Stdin.Read()Write()delegates tom.f.Write()instead ofos.Stdout.Write()Fd()returnsm.f.Fd()instead of the hardcoded stdin handleName()returnsm.f.Name()instead of a hardcoded stringnewMaster()storesfin the structBehavior change —
Name():On Windows,
Name()previously returned a fixed"console"string. It now returns the name of the underlying file the console was created from (for example"/dev/stdin"/"/dev/stdout"), consistent with the Unix implementation. Callers that depended on the literal"console"value should be aware of this change.Note on console-mode operations:
The console-mode operations (
SetRaw,Reset,Size,DisableEcho) still act on the process's standard handles rather thanfalone. On Windows the standard streams share a single underlying console object, so mode and size queries apply to that console as a whole. This is documented onnewMaster.Tests:
TestMaster_DelegatesToFile: in-process, pipe-backed test verifyingmasterdelegatesWriteandReadto its file (the regression test for Windows: stderr output leaks into stdout #83, where output written to a console created from a non-stdout stream leaked to stdout), plus assertions thatFd()/Name()track the provided file andFd()is notos.Stdout's descriptor. Runs anywhere — no attached console required.TestConsoleFromFile_Delegation: end-to-end subprocess test usingCONIN$/CONOUT$to exercise the public API with real console handles; the subprocess's stderr is captured so failures surface with detail.Fixes #83