fix(ext/node): validate fd in tty.isatty and enable pseudo-tty tests #31892
Conversation
fraidev
changed the title
fraidev
force-pushed
the
fix/node-tty-isatty-integer-validation
branch
2 times, most recently
from
bb85356 to
63ada3e
Compare
fraidev
force-pushed
the
fix/node-tty-isatty-integer-validation
branch
3 times, most recently
from
b4f4f08 to
41e80bc
Compare
fraidev
changed the title
fraidev
force-pushed
the
fix/node-tty-isatty-integer-validation
branch
from
41e80bc to
8ea451c
Compare
| } | ||
|
|
||
| #[cfg(target_os = "windows")] | ||
| fn run_in_pty_impl( |
There was a problem hiding this comment.
I'm confused - weren't tests supposed to be skipped on Windows? If so then why do we need this impl?
There was a problem hiding this comment.
It should run on local Windows, just windows CI do not support PTY.
I didn't test on Windows locally, though
There was a problem hiding this comment.
I tested it on local Windows and is working fine.
There was a problem hiding this comment.
Pull request overview
This PR fixes the tty.isatty() function to properly validate file descriptor values by returning false for negative and non-integer inputs, aligning with Node.js behavior. Additionally, it introduces PTY infrastructure to enable running pseudo-tty tests in real terminal environments.
Changes:
- Fixed
tty.isatty()to validate that fd is a non-negative integer using the same pattern as existing ReadStream/WriteStream constructors - Added comprehensive tests for the new validation behavior including negative and fractional fd values
- Implemented
run_in_pty()infrastructure for both Unix and Windows to execute tests in real PTY environments - Modified node_compat test runner to detect and execute pseudo-tty tests using the new PTY infrastructure
- Enabled the
pseudo-tty/test-tty-isatty.jstest in the node compatibility test suite
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| ext/node/polyfills/tty.js | Added fd validation to check for non-negative integers in isatty() |
| tests/unit_node/tty_test.ts | Added tests for negative and non-integer fd validation |
| tests/util/server/src/pty.rs | Implemented run_in_pty() with PtyOutput struct for Unix and Windows platforms |
| tests/node_compat/mod.rs | Added PTY detection and execution path for pseudo-tty tests |
| tests/node_compat/config.jsonc | Enabled pseudo-tty/test-tty-isatty.js test |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut child = unsafe { | ||
| std::process::Command::new(program) | ||
| .current_dir(cwd) | ||
| .args(args) | ||
| .envs(env_vars.unwrap_or_default()) | ||
| .pre_exec(move || { | ||
| // Create a new session | ||
| libc::setsid(); | ||
| // Set the controlling terminal | ||
| libc::ioctl(fds, libc::TIOCSCTTY as libc::c_ulong, 0); | ||
| set_winsize(fds, PTY_ROWS_COLS.0, PTY_ROWS_COLS.1)?; | ||
| // Close parent's main handle | ||
| libc::close(fdm); | ||
| libc::dup2(fds, 0); | ||
| libc::dup2(fds, 1); | ||
| libc::dup2(fds, 2); | ||
| libc::close(fds); | ||
| Ok(()) | ||
| }) | ||
| .spawn() | ||
| .unwrap() | ||
| }; |
There was a problem hiding this comment.
If spawn() fails and panics on line 403, the file descriptors fdm and fds will be leaked. Consider using RAII guards (e.g., a custom struct with Drop implementation) or handling the error gracefully to ensure file descriptors are closed even on failure. While this is test infrastructure, proper cleanup would make debugging test failures easier.
2 participants
Closes #29876