Skip to content

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

Merged
vstinner merged 2 commits intopython:mainfrom
vstinner:socket_timeout
Nov 19, 2024
Merged

gh-126876: Fix socket internal_select() for large timeout#126968
vstinner merged 2 commits intopython:mainfrom
vstinner:socket_timeout

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 only 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
Closed
@vstinner
Copy link
Member Author

vstinner commented Nov 18, 2024

cc @sobolevn

@vstinner
Copy link
Member Author

vstinner commented Nov 18, 2024

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
/* 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

@sobolevn sobolevn Nov 18, 2024

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
@vstinner vstinner deleted the socket_timeout branch November 19, 2024 08:08
@miss-islington-app
Copy link

miss-islington-app bot commented Nov 19, 2024

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 only 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>
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
@vstinner @ebonnal
pythongh-126876: Fix socket internal_select() for large timeout (pyth…
4c36f2b 
…on#126968)

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

Add an unit test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sobolevn sobolevn sobolevn approved these changes

+1 more reviewer

@ruidazeng ruidazeng ruidazeng approved these changes

Reviewers whose approvals may not affect merge requirements

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