Uh oh!
There was an error while loading. Please reload this page.
gh-149816: Fix SNI callback callable race - #150018
Conversation
All commit authors signed the Contributor License Agreement. |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
In Modules/_ssl.c
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
| if (!PyCallable_Check(value)) { | ||
| SSL_CTX_set_tlsext_servername_callback(self->ctx, NULL); | ||
| } | ||
| else { | ||
| if (!PyCallable_Check(value)) { | ||
| SSL_CTX_set_tlsext_servername_callback(self->ctx, NULL); | ||
| PyErr_SetString(PyExc_TypeError, | ||
| "not a callable object"); | ||
| Py_CLEAR(self->set_sni_cb); | ||
| if (value != Py_None) { | ||
| PyErr_SetString(PyExc_TypeError, "not a callable object"); | ||
| return -1; | ||
| } | ||
| self->set_sni_cb = Py_NewRef(value); | ||
| SSL_CTX_set_tlsext_servername_callback(self->ctx, _servername_callback); | ||
| } | ||
| else { | ||
| Py_INCREF(value); | ||
| PyObject *old_cb = _Py_atomic_exchange_ptr(&self->set_sni_cb, value); | ||
| Py_XDECREF(old_cb); | ||
| SSL_CTX_set_tlsext_servername_arg(self->ctx, self); | ||
| SSL_CTX_set_tlsext_servername_callback(self->ctx, _servername_callback); |
There was a problem hiding this comment.
These changes look unnecessary if set_sni_cb is always accessed in a critical section.
There was a problem hiding this comment.
Unfortunately just the critical section didn't help (thread sanitizer kept complaining). Probably because it's getting accessed from within ssl library itself, so we can't guard against that.
There was a problem hiding this comment.
Never mind, I think I got it working
Uh oh!
There was an error while loading. Please reload this page.
Sorry, @kiri11 and @encukou, I could not cleanly backport this to |
GH-150099 is a backport of this pull request to the 3.15 branch. |
GH-150100 is a backport of this pull request to the 3.14 branch. |
This PR does not fixes the internal data races in openssl, see #150191 (comment) for the details of the race. For now I have skipped this test under TSAN to keep TSAN CI working but this needs to be either fixed properly or documented as not thread safe as most other ssl operations. cc @encukou |
Problem
Fix a use-after-free race in the SSL SNI callback on free-threaded builds. When
sni_callbackis replaced or cleared on one thread while another thread is mid-handshake, the old callback object could be freed before the handshake thread finishes calling it._servername_callback): acquiresslctx's critical section, snapshot the callback withPy_XNewRefinto a local, then release. The local strong reference keeps the callable alive for the duration of the call.sni_callback_set): already holds the same critical section via@critical_sectionclinic annotation. UsesPy_XSETREFto swap the pointer and decref the old callback atomically. The OpenSSL callback registration is set after the pointer is stored, so a concurrent handshake always finds a valid callable.sni_callback = None): unregisters the OpenSSL callback first, then clears the Python reference. Serialized against the reader by the same critical section.Testing
Adds a free-threading stress test that spawns handshake workers and a callback-toggling thread concurrently.
Plus manually tested on MacOS.
Before
After
Using Repro:
More info