Fix Windows is_terminal() for write-only console handles (fixes #130974)#153219
Fix Windows is_terminal() for write-only console handles (fixes #130974)#153219shri-prakhar wants to merge 1 commit intorust-lang:mainfrom
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
|
r? @ChrisDenton (Or if you don't have bandwidth we can ping Windows group) |
|
|
There was a problem hiding this comment.
Although this code of smells of AI generation, nothing looks obviously incorrect. A lot of the comments are redundant or highly verbose (in view of the assert messages and just test names).
Also, the asserts and the test specific comments should document expected behaviour, panic messages are redundant and can be removed.
library/std/tests/is_terminal.rs
Outdated
| ); | ||
| } | ||
| Err(_) => { | ||
| // No console attached (headless CI, service, etc.) — skip. |
There was a problem hiding this comment.
Should we be blindly eating the case where \\.\CONOUT$ doesn't exist? This could mask away potential regressions if there is no conhost attached during testing.
The test should (ideally) create a conhost if it's not already attached to one.
There was a problem hiding this comment.
@Teapot4195 Fair points all around. The verbose comments were AI-generated; I leaned on it for
documentation while I was working through the Windows console API surface, and I wasn't aware that
went against the review policy. That's on me. I used AI to help scaffold the tests documentation and comments
and that resulted in the overly verbose comments and redundant assert messages.
I should have cleaned the output more carefully before submitting lesson learned.
I'll strip the comments, remove the panic strings, and let the test names and
assertions speak for themselves.
On the \\.\CONOUT$ skip — agreed, silently eating Err(_) could hide
regressions in headless CI. I'll rework it so the test ensures a console
exists (e.g. via AllocConsole if not already attached) rather than bailing.
Will push a cleaned-up revision. Thanks for the thorough review.
|
Are consoles really the only character-device handles where |
|
@joboet Based on the Windows docs, yes The key distinction is how the kernel dispatches console API calls. This is consistent with the Console Buffer Security docs, where the "wrong-way verbs" I verified this with NUL ( That said, the docs don't publish a formal error code table for |
Made-with: Cursor
bc69769 to
163c74d
Compare
|
Pushed a revision. |
On Windows,
is_terminal()was incorrect for console handles opened without read access (e.g. write-onlyCONOUT$).GetConsoleModeneedsGENERIC_READ; without it the API fails withERROR_ACCESS_DENIEDinstead of reporting a console. The code treated that as “not a terminal,” so write-only console handles were misclassified.This change fixes that by treating
ERROR_ACCESS_DENIEDfromGetConsoleModeas “might be a console” and then usingGetFileType: console buffers areFILE_TYPE_CHAR. The NUL device is alsoFILE_TYPE_CHARbutGetConsoleModereturnsERROR_INVALID_HANDLEfor it, so NUL never hits this branch and is still correctly reported as non-terminal.Fixes #130974.
Changes
library/std/src/sys/io/is_terminal/windows.rs: AfterGetConsoleModefails, check forERROR_ACCESS_DENIEDand, in that case, confirm a console withGetFileType(handle) == FILE_TYPE_CHAR. Added a short comment explaining the logic and the NUL exception.library/std/tests/is_terminal.rs: New Windows-only tests forIsTerminal:CONOUT$(On Windowsis_terminalalways returnsfalseif the handle is not opened with read access #130974).CONOUT$and read-onlyCONIN$.stdin/stdout/stderris_terminal()calls do not panic.Testing
python x.py test library/std(including the newis_terminaltests).CONOUT$handle to confirm it is now reported as a terminal.