| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import socket | ||
| import unittest | ||
|
|
||
| from test import support | ||
| from test.support import threading_helper | ||
|
|
||
| N = 200 if support.check_sanitizer(thread=True) else 1000 | ||
|
|
||
|
|
||
| @threading_helper.requires_working_threading() | ||
| class TestSocketTimeoutRaces(unittest.TestCase): | ||
| # gh-153852: the socket timeout was read/written non-atomically, so reading | ||
| # it with gettimeout()/getblocking() raced with settimeout()/setblocking() | ||
| # on the same socket. | ||
|
|
||
| def new_socket(self): | ||
| s = socket.socket() | ||
| self.addCleanup(s.close) | ||
| return s | ||
|
|
||
| def check_race(self, read, write): | ||
| def reader(): | ||
| for _ in range(N): | ||
| read() | ||
|
|
||
| def writer(): | ||
| for _ in range(N): | ||
| write() | ||
|
|
||
| threading_helper.run_concurrently([reader, writer]) | ||
|
|
||
| def test_gettimeout_vs_setblocking(self): | ||
| s = self.new_socket() | ||
| self.check_race(s.gettimeout, lambda: s.setblocking(False)) | ||
|
|
||
| def test_gettimeout_vs_settimeout(self): | ||
| s = self.new_socket() | ||
| self.check_race(s.gettimeout, lambda: s.settimeout(1.0)) | ||
|
|
||
| def test_getblocking_vs_settimeout(self): | ||
| s = self.new_socket() | ||
| self.check_race(s.getblocking, lambda: s.settimeout(0)) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| unittest.main() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Fix a data race on a socket's timeout in the :term:`free-threaded build`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| Expand Up | @@ -110,6 +110,7 @@ Local naming conventions: | |
| #include "pycore_fileutils.h" // _Py_set_inheritable() | ||
| #include "pycore_moduleobject.h" // _PyModule_GetState | ||
| #include "pycore_object.h" // _PyObject_VisitType() | ||
| #include "pycore_pyatomic_ft_wrappers.h" // FT_ATOMIC_LOAD_INT64_ACQUIRE() | ||
| #include "pycore_time.h" // _PyTime_AsMilliseconds() | ||
| #include "pycore_tuple.h" // _PyTuple_FromPairSteal | ||
| #include "pycore_pystate.h" // _Py_AssertHoldsTstate() | ||
| Expand Down Expand Up | @@ -591,6 +592,12 @@ get_sock_fd(PySocketSockObject *s) | |
| #endif | ||
| } | ||
|
|
||
| static inline PyTime_t | ||
| get_sock_timeout(PySocketSockObject *s) | ||
| { | ||
| return FT_ATOMIC_LOAD_INT64_ACQUIRE(s->sock_timeout); | ||
| } | ||
|
|
||
| #define _PySocketSockObject_CAST(op) ((PySocketSockObject *)(op)) | ||
|
|
||
| static inline socket_state * | ||
| Expand Down Expand Up | @@ -681,7 +688,7 @@ class _socket.socket "PySocketSockObject *" "clinic_state()->sock_type" | |
| #else | ||
| /* If there's no timeout left, we don't have to call select, so it's a safe, | ||
| * little white lie. */ | ||
| #define IS_SELECTABLE(s) (_PyIsSelectable_fd((s)->sock_fd) || (s)->sock_timeout <= 0) | ||
|
Comment thread
Copy link
Copy Markdown
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThese shouldn't be atomic on the GILicious build. Add a wrapper to pycore_pyatomic_ft_wrappers.h and use that here.
Sorry, something went wrong.
deadlovelll reacted with thumbs up emoji
All reactions
Copy link
Copy Markdown
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityThanks for the review, I'll keep that in mind for the future. I've pushed the changes
Sorry, something went wrong.
All reactions
|
||
| #define IS_SELECTABLE(s) (_PyIsSelectable_fd((s)->sock_fd) || get_sock_timeout(s) <= 0) | ||
| #endif | ||
|
|
||
| // SCM_RIGHTS, sendmsg(), recvmsg() and sethostname() don't work properly on | ||
| Expand Down Expand Up | @@ -1068,7 +1075,7 @@ sock_call_ex(PySocketSockObject *s, | |
| /* retry sock_func() */ | ||
| } | ||
|
|
||
| if (s->sock_timeout > 0 | ||
| if (get_sock_timeout(s) > 0 | ||
| && (CHECK_ERRNO(EWOULDBLOCK) || CHECK_ERRNO(EAGAIN))) { | ||
| /* False positive: sock_func() failed with EWOULDBLOCK or EAGAIN. | ||
|
|
||
| Expand All | @@ -1094,7 +1101,8 @@ sock_call(PySocketSockObject *s, | |
| int (*func) (PySocketSockObject *s, void *data), | ||
| void *data) | ||
| { | ||
| return sock_call_ex(s, writing, func, data, 0, NULL, s->sock_timeout); | ||
| return sock_call_ex(s, writing, func, data, 0, NULL, | ||
| get_sock_timeout(s)); | ||
| } | ||
|
|
||
|
|
||
| Expand Down Expand Up | @@ -1158,6 +1166,9 @@ new_sockobject(socket_state *state, SOCKET_T fd, int family, int type, | |
| if (s == NULL) { | ||
| return NULL; | ||
| } | ||
| #ifdef Py_GIL_DISABLED | ||
| s->sock_timeout_mutex = (PyMutex){0}; | ||
| #endif | ||
| if (init_sockobject(state, s, fd, family, type, proto) == -1) { | ||
| Py_DECREF(s); | ||
| return NULL; | ||
| Expand Down Expand Up | @@ -3168,9 +3179,15 @@ sock_setblocking(PyObject *self, PyObject *arg) | |
| if (block < 0) | ||
| return NULL; | ||
|
|
||
| PySocketSockObject *s = _PySocketSockObject_CAST(self); | ||
| s->sock_timeout = _PyTime_FromSeconds(block ? -1 : 0); | ||
| if (internal_setblocking(s, block) == -1) { | ||
| PySocketSockObject *s = _PySocketSockObject_CAST(self); | ||
| FT_MUTEX_LOCK(&s->sock_timeout_mutex); | ||
| int result = internal_setblocking(s, block); | ||
| if (result == 0) { | ||
| FT_ATOMIC_STORE_INT64_RELEASE( | ||
| s->sock_timeout, _PyTime_FromSeconds(block ? -1 : 0)); | ||
| } | ||
| FT_MUTEX_UNLOCK(&s->sock_timeout_mutex); | ||
| if (result < 0) { | ||
| return NULL; | ||
| } | ||
| Py_RETURN_NONE; | ||
| Expand All | @@ -3190,8 +3207,8 @@ setblocking(False) is equivalent to settimeout(0.0)."); | |
| static PyObject * | ||
| sock_getblocking(PyObject *self, PyObject *Py_UNUSED(ignored)) | ||
| { | ||
| PySocketSockObject *s = _PySocketSockObject_CAST(self); | ||
| if (s->sock_timeout) { | ||
| PySocketSockObject *s = _PySocketSockObject_CAST(self); | ||
| if (get_sock_timeout(s)) { | ||
| Py_RETURN_TRUE; | ||
| } | ||
| else { | ||
| Expand Down Expand Up | @@ -3261,8 +3278,6 @@ sock_settimeout(PyObject *self, PyObject *arg) | |
| return NULL; | ||
|
|
||
| PySocketSockObject *s = _PySocketSockObject_CAST(self); | ||
| s->sock_timeout = timeout; | ||
|
|
||
| int block = timeout < 0; | ||
| /* Blocking mode for a Python socket object means that operations | ||
| like :meth:`recv` or :meth:`sendall` will block the execution of | ||
| Expand All | @@ -3285,7 +3300,13 @@ sock_settimeout(PyObject *self, PyObject *arg) | |
| ``> 0`` ``True`` non-blocking | ||
| */ | ||
|
|
||
| if (internal_setblocking(s, block) == -1) { | ||
| FT_MUTEX_LOCK(&s->sock_timeout_mutex); | ||
| int result = internal_setblocking(s, block); | ||
| if (result == 0) { | ||
| FT_ATOMIC_STORE_INT64_RELEASE(s->sock_timeout, timeout); | ||
| } | ||
| FT_MUTEX_UNLOCK(&s->sock_timeout_mutex); | ||
| if (result < 0) { | ||
| return NULL; | ||
| } | ||
| Py_RETURN_NONE; | ||
| Expand All | @@ -3305,11 +3326,12 @@ static PyObject * | |
| sock_gettimeout_impl(PyObject *self, void *Py_UNUSED(ignored)) | ||
| { | ||
| PySocketSockObject *s = _PySocketSockObject_CAST(self); | ||
| if (s->sock_timeout < 0) { | ||
| PyTime_t sock_timeout = get_sock_timeout(s); | ||
| if (sock_timeout < 0) { | ||
| Py_RETURN_NONE; | ||
| } | ||
| else { | ||
| double seconds = PyTime_AsSecondsDouble(s->sock_timeout); | ||
| double seconds = PyTime_AsSecondsDouble(sock_timeout); | ||
| return PyFloat_FromDouble(seconds); | ||
| } | ||
| } | ||
| Expand Down Expand Up | @@ -3706,10 +3728,11 @@ internal_connect(PySocketSockObject *s, struct sockaddr *addr, int addrlen, | |
| If the socket is non-blocking, raise InterruptedError. The caller is | ||
| responsible to wait until the connection completes, fails or timed | ||
| out (it's the case in asyncio for example). */ | ||
| wait_connect = (s->sock_timeout != 0 && IS_SELECTABLE(s)); | ||
| wait_connect = (get_sock_timeout(s) != 0 && IS_SELECTABLE(s)); | ||
| } | ||
| else { | ||
| wait_connect = (s->sock_timeout > 0 && err == SOCK_INPROGRESS_ERR | ||
| wait_connect = (get_sock_timeout(s) > 0 | ||
| && err == SOCK_INPROGRESS_ERR | ||
| && IS_SELECTABLE(s)); | ||
| } | ||
|
|
||
| Expand All | @@ -3727,13 +3750,13 @@ internal_connect(PySocketSockObject *s, struct sockaddr *addr, int addrlen, | |
| if (raise) { | ||
| /* socket.connect() raises an exception on error */ | ||
| if (sock_call_ex(s, 1, sock_connect_impl, NULL, | ||
| 1, NULL, s->sock_timeout) < 0) | ||
| 1, NULL, get_sock_timeout(s)) < 0) | ||
| return -1; | ||
| } | ||
| else { | ||
| /* socket.connect_ex() returns the error code on error */ | ||
| if (sock_call_ex(s, 1, sock_connect_impl, NULL, | ||
| 1, &err, s->sock_timeout) < 0) | ||
| 1, &err, get_sock_timeout(s)) < 0) | ||
| return err; | ||
| } | ||
| return 0; | ||
| Expand Down Expand Up | @@ -4688,8 +4711,8 @@ _socket_socket_sendall_impl(PySocketSockObject *s, Py_buffer *pbuf, | |
| char *buf; | ||
| Py_ssize_t len, n; | ||
| struct sock_send ctx; | ||
| int has_timeout = (s->sock_timeout > 0); | ||
| PyTime_t timeout = s->sock_timeout; | ||
| PyTime_t timeout = get_sock_timeout(s); | ||
| int has_timeout = (timeout > 0); | ||
| PyTime_t deadline = 0; | ||
| int deadline_initialized = 0; | ||
| PyObject *res = NULL; | ||
| Expand Down Expand Up | @@ -5604,6 +5627,9 @@ sock_new(PyTypeObject *type, PyObject *args, PyObject *kwds) | |
| if (new != NULL) { | ||
| ((PySocketSockObject *)new)->sock_fd = INVALID_SOCKET; | ||
| ((PySocketSockObject *)new)->sock_timeout = _PyTime_FromSeconds(-1); | ||
| #ifdef Py_GIL_DISABLED | ||
| ((PySocketSockObject *)new)->sock_timeout_mutex = (PyMutex){0}; | ||
| #endif | ||
| ((PySocketSockObject *)new)->errorhandler = &set_error; | ||
| #ifdef MS_WINDOWS | ||
| ((PySocketSockObject *)new)->quickack = 0; | ||
| Expand Down | ||
| Back | FazBrowse Home | New Git URL |
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low QualityFWIW I don't think it is worth adding tests for this, initially when I worked on fixing races on socket fd I didn't add tests because they won't be exhaustive and the race is trivial.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Qualityhello, there's a tsan ci job that runs test_free_threading, so this test is a regression guard here, thats why i added it. but i'd be happy to remove it if you'd rather not have it
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.