-
Notifications
You must be signed in to change notification settings - Fork 383
handle TIOCGWINSZ struct correctly #1040
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Python 3.14 just released, and |
Hi 👋 I think the actual issue here is the struct is incorrectly sized. https://www.man7.org/linux/man-pages/man2/TIOCSWINSZ.2const.html struct winsize {
unsigned short ws_row;
unsigned short ws_col;
unsigned short ws_xpixel; /* unused */
unsigned short ws_ypixel; /* unused */
}; So to fix this I think maybe we need to change the code to something like this. fmt = "HHHH" # <- 4 unsigned shorts
buf = struct.pack(fmt, 0, 0, 0, 0) # <- Provide 4 zeros here
try:
result = fcntl.ioctl(sys.stdout, termios.TIOCGWINSZ, buf)
rows, cols, *_ = struct.unpack(fmt, result) # <- Ignore the ws_xpixel and ws_ypixel
return (cols, rows) |
`TIOCGWINSZ` contains 4, not 2 unsigned shorts: ```C struct winsize { unsigned short ws_row; unsigned short ws_col; unsigned short ws_xpixel; /* unused */ unsigned short ws_ypixel; /* unused */ }; ``` Fixes the following test failures when building with Python3.14: ``` =========================== short test summary info ============================ FAILED tests/runners.py::Local_::pty::when_pty_True_we_use_pty_fork_and_os_exec - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::pty_uses_WEXITSTATUS_if_WIFEXITED - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::pty_uses_WTERMSIG_if_WIFSIGNALED - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::WTERMSIG_result_turned_negative_to_match_subprocess - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::pty_is_set_to_controlling_terminal_size - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::spurious_OSErrors_handled_gracefully - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::other_spurious_OSErrors_handled_gracefully - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::non_spurious_OSErrors_bubble_up - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::stop_mutes_errors_on_pty_close - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::fallback::can_be_overridden_by_kwarg - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::fallback::can_be_overridden_by_config - SystemError: buffer overflow FAILED tests/runners.py::Local_::pty::fallback::overridden_fallback_affects_result_pty_value - SystemError: buffer overflow FAILED tests/runners.py::Local_::shell::defaults_to_bash_or_cmdexe_when_pty_True - SystemError: buffer overflow FAILED tests/runners.py::Local_::shell::may_be_overridden_when_pty_True - SystemError: buffer overflow FAILED tests/runners.py::Local_::env::uses_execve_for_pty_True - SystemError: buffer overflow FAILED tests/terminals.py::terminals::pty_size::calls_fcntl_with_TIOCGWINSZ - SystemError: buffer overflow ================== 16 failed, 952 passed, 11 skipped in 6.67s ================== ``` Resolves pyinvoke#1038 .
SystemError
exception
Great catch, thanks!
PR updated. How do you want your contribution to be acknowledged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Don't worry about that 😄 |
Thanks for posting this! Integrating now. Also praying that the fix (assume the struct has 4 members, as is man-paged, then just ignore all but the first 2 results) doesn't backfire in other scenarios somehow. AFAICT this has been the case for many years and the original approach just came from the very olden days or was somebody trying to be clever. I'm curious if anyone can track down exactly why this changed, the 3.14 changelog linked in #1038 doesn't actually seem to include a line item that would cause this (to my skimming eyes). EDIT: ah, here it is, python/cpython#132915 - unpacking a 4-field struct as if it were only the first 2 fields does technically qualify as a buffer overflow, I guess. |
Backporting this to 2.1 and 2.2, but not 2.0 as there's an annoying merge conflict. Should be pushing 2.1.4 and 2.2.1 to PyPI in a few. Will appear in main due to merging up, but as usual, Github can't tell so thinks this "isn't merged" 😩 EDIT: CI is stuffed at the moment, trying to support 3.6->3.14 is way too big an ask, the ecosystem cannae handle it, Captain! I have an in-progress branch from earlier this year updating us to use modern tooling expectations (3.9+, uv, basically) based partly on work I have released in at least one other project. Going to publish this bugfix to PyPI now and push that branch to completion over the weekend, with aim of "2.1/2.2 branch lines will show 3.7-3.8 working, and main will show 3.9+ working" as the band-aid. EDIT 2: ok 2.1/2.2 branches have slightly less borked CI capped at Python 3.11, and main branch is now upgraded fully for pyproject.toml/Pythons 3.9-3.14(ish - CircleCI has no 3.14 image yet). |
Invoke 2.1.4 and 2.2.1 are on PyPI and "work on my machine" (new venv, pip install either version, do basic invoke things) again under Python 3.8 and 3.14. Thanks all! |
TIOCGWINSZ
contains 4, not 2 unsigned shorts:Fixes the following test failures when building with Python3.14:
Resolves #1038 .