Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-126876: Fix socket internal_select() for large timeout #126968

Merged
merged 2 commits into from
Nov 19, 2024
Merged

gh-126876: Fix socket internal_select() for large timeout #126968

merged 2 commits into from
Nov 19, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Nov 18, 2024 •
edited by bedevere-app bot
Loading

If the timeout is larger than INT_MAX, replace it with INT_MAX, in the poll() code path.

Add an unit test.

@vstinner
pythongh-126876: Fix socket internal_select() for large timeout
23f99f5 
If the timeout is larger than INT_MAX, replace it with INT_MAX, in
the poll() code path.

Add an unit test.
@vstinner vstinner added skip news needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Nov 18, 2024
@bedevere-app bedevere-app bot mentioned this pull request Nov 18, 2024
Open
@vstinner
Copy link
Member Author

cc @sobolevn

@vstinner
Copy link
Member Author

With this change, select() and poll() code paths have the same behavior: saturate the timeout to the implementation maximum.

sobolevn
sobolevn approved these changes Nov 18, 2024
View reviewed changes
Modules/socketmodule.c
@@ -810,7 +810,9 @@ internal_select(PySocketSockObject *s, int writing, PyTime_t interval,

/* s->sock_timeout is in seconds, timeout in ms */
ms = _PyTime_AsMilliseconds(interval, _PyTime_ROUND_CEILING);
assert(ms <= INT_MAX);
if (ms > INT_MAX) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indeed my proposal was wrong. Because later ms is casted to int: n = poll(&pollfd, 1, (int)ms);

Thanks for picking this up!

@vstinner
Copy link
Member Author

vstinner commented Nov 18, 2024 •
edited
Loading

The test fails on Windows:

======================================================================
ERROR: testLargeTimeout (test.test_socket.NonBlockingTCPTests.testLargeTimeout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_socket.py", line 5144, in testLargeTimeout
    conn.settimeout(large_timeout)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
OverflowError: timeout doesn't fit into C timeval

======================================================================
ERROR: testLargeTimeout (test.test_socket.NonBlockingTCPTests.testLargeTimeout)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_socket.py", line 383, in raise_queued_exception
    raise self.queue.get()
  File "D:\a\cpython\cpython\Lib\test\test_socket.py", line 420, in clientRun
    test_func()
    ~~~~~~~~~^^
  File "D:\a\cpython\cpython\Lib\test\test_socket.py", line 5151, in _testLargeTimeout
    self.cli.settimeout(large_timeout)
    ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
OverflowError: timeout doesn't fit into C timeval

I modified the test to skip the test if settimeout() raises OverflowError (on Windows).

@vstinner vstinner merged commit b3687ad into python:main Nov 19, 2024
37 checks passed
@vstinner vstinner deleted the socket_timeout branch November 19, 2024 08:08
@miss-islington-app
Copy link

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 19, 2024
@vstinner @miss-islington
pythongh-126876: Fix socket internal_select() for large timeout (pyth…
f693004 
…onGH-126968)

If the timeout is larger than INT_MAX, replace it with INT_MAX, in
the poll() code path.

Add an unit test.
(cherry picked from commit b3687ad)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 19, 2024
@vstinner @miss-islington
pythongh-126876: Fix socket internal_select() for large timeout (pyth…
e51cc5b 
…onGH-126968)

If the timeout is larger than INT_MAX, replace it with INT_MAX, in
the poll() code path.

Add an unit test.
(cherry picked from commit b3687ad)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-app
Copy link

bedevere-app bot commented Nov 19, 2024

GH-127002 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 19, 2024
@bedevere-app
Copy link

bedevere-app bot commented Nov 19, 2024

GH-127003 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Nov 19, 2024
vstinner added a commit that referenced this pull request Dec 2, 2024
@miss-islington @vstinner
[3.13] gh-126876: Fix socket internal_select() for large timeout (GH-…
059114c 
…126968) (#127002)

gh-126876: Fix socket internal_select() for large timeout (GH-126968)

If the timeout is larger than INT_MAX, replace it with INT_MAX, in
the poll() code path.

Add an unit test.
(cherry picked from commit b3687ad)

Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner added a commit that referenced this pull request Dec 3, 2024
@miss-islington @vstinner
[3.12] gh-126876: Fix socket internal_select() for large timeout (GH-…
b49e902 
…126968) (#127003)

gh-126876: Fix socket internal_select() for large timeout (GH-126968)

If the timeout is larger than INT_MAX, replace it with INT_MAX, in
the poll() code path.

Add an unit test.
(cherry picked from commit b3687ad)

Co-authored-by: Victor Stinner <vstinner@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruidazeng ruidazeng ruidazeng approved these changes

@sobolevn sobolevn sobolevn approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vstinner @sobolevn @ruidazeng