bpo-30594: Fix spurious DECREF in newPySSLSocket #1992
Conversation
In newPySSLSocket, it sets up the new 'self' object, which among other things owns a reference to the parent SSLContext in 'self->ctx'. It then tries to idna-decode the given server_hostname, and if this fails it does Py_DECREF(self) and returns. The Py_DECREF(self) causes the PySSLSocket destructor to run, which calls Py_DECREF(self->ctx), which releases the reference to the parent SSLContext object. However... as currently written, we don't actually *take* the reference to the parent SSLContext until *after* the idna-decoding step. I.e., this does a Py_DECREF on an object that was never Py_INCREFed. Eventually we get a segfault.
|
@njsmith, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vadmium, @tiran and @serhiy-storchaka to be potential reviewers. |
njsmith
changed the title
|
Of course, the real solution would be to not attempt to IDNA-decode the passed in name, why are we even doing this, everything inside this module should be using A-labels. But that's another story... |
serhiy-storchaka
added
needs backport to 2.7
type-bug
|
Would it be possible to write an unit test? |
|
@Haypo: The truth is I couldn't figure out where to put such a test (there are currently literally zero tests for anything related to IDN and ssl), and wasn't sure what the preferred style was for refcount issues like this anyway. (Call it 100 times and then move on, and if the tests segfault then that's a failure?) |
|
Sorry, hit post too soon and lost my last sentence :-). What I meant to say is, if you can tell me the answer to those I can write a test :-) |
If pass a server_hostname= that fails IDNA decoding to SSLContext.wrap_socket or SSLContext.wrap_bio, then the SSLContext object had a spurious Py_DECREF called on it, eventually leading to segfaults. (cherry picked from commit 65ece7c)
If pass a server_hostname= that fails IDNA decoding to SSLContext.wrap_socket or SSLContext.wrap_bio, then the SSLContext object had a spurious Py_DECREF called on it, eventually leading to segfaults. (cherry picked from commit 65ece7c)
|
This is trivial fix, and it is very unlikely the bug will be reintroduced. I think a test and a Misc/NEWS entry are not required for this issue. |
|
@njsmith: Try to write an unit test in test_ssl on SSLContext with an invalid IDNA. @serhiy-storchaka: I agree that the unit test is not required, but it would be nice to test the error path as well ;-) |
If pass a server_hostname= that fails IDNA decoding to SSLContext.wrap_socket or SSLContext.wrap_bio, then the SSLContext object had a spurious Py_DECREF called on it, eventually leading to segfaults. (cherry picked from commit 65ece7c)
If pass a server_hostname= that fails IDNA decoding to SSLContext.wrap_socket or SSLContext.wrap_bio, then the SSLContext object had a spurious Py_DECREF called on it, eventually leading to segfaults. (cherry picked from commit 65ece7c)
In newPySSLSocket, it sets up the new 'self' object, which among other
things owns a reference to the parent SSLContext in 'self->ctx'.
It then tries to idna-decode the given server_hostname, and if this
fails it does Py_DECREF(self) and returns.
The Py_DECREF(self) causes the PySSLSocket destructor to run, which
calls Py_DECREF(self->ctx), which releases the reference to the parent
SSLContext object.
However... as currently written, we don't actually take the
reference to the parent SSLContext until after the idna-decoding
step. I.e., this does a Py_DECREF on an object that was never
Py_INCREFed. Eventually we get a segfault.