gh-83004: Clean up refleaks in ssl, socket initialisation #99044
Conversation
hauntsaninja
commented
Nov 3, 2022
•
hauntsaninja
commented
- Issue: Improve stdlib module initialization error handling. #83004
| @@ -5925,8 +5925,7 @@ sslmodule_init_constants(PyObject *m) | |||
| #define addbool(m, key, value) \ | |||
| do { \ | |||
| PyObject *bool_obj = (value) ? Py_True : Py_False; \ | |||
| Py_INCREF(bool_obj); \ | |||
| PyModule_AddObject((m), (key), bool_obj); \ | |||
| PyModule_AddObjectRef((m), (key), bool_obj); \ | |||
There was a problem hiding this comment.
Isn't this still a leak because we're not checking the return value?
There was a problem hiding this comment.
It's no longer a leak.
The thing that makes PyModule_AddObject tricky cause leaks is that you have to decref in the failure case, but not in the success case (because it steals the ref). With PyModule_AddObjectRef you can treat the failure and success cases the same in terms of what they do to ref counts.
But yes, in general we should probably be propagating failures here, and the other cases below you commented on. I didn't do it in this PR because it's a slightly separate change and there are many other places in this module where we don't check return values.
There was a problem hiding this comment.
Should I check return values for just the ones I'm touching in this PR or fix all of them?
There was a problem hiding this comment.
Thanks for the explanation! I feel it'd be fine to also fix missing error checks in this PR, since the change remains localized and you're fixing a very similar problem to what the PR already fixes.
| return NULL; | ||
|
|
||
| #ifdef ENABLE_IPV6 | ||
| has_ipv6 = Py_True; | ||
| #else | ||
| has_ipv6 = Py_False; | ||
| #endif | ||
| Py_INCREF(has_ipv6); | ||
| PyModule_AddObject(m, "has_ipv6", has_ipv6); | ||
| PyModule_AddObjectRef(m, "has_ipv6", has_ipv6); |
| @@ -8666,7 +8662,8 @@ PyInit__socket(void) | |||
| tmp = PyLong_FromUnsignedLong(codes[i]); | |||
| if (tmp == NULL) | |||
| return NULL; | |||
| PyModule_AddObject(m, names[i], tmp); | |||
| PyModule_AddObjectRef(m, names[i], tmp); | |||
|
The socket issues have been resolved in gh-103261. |
|
Closing, as the issues have been resolved in other PRs. Thanks :) |
