bpo-34408: Prevent a null pointer derefence and resource leakage in pystate.c#8767
Conversation
| "failed to get an interpreter ID"); | ||
| /* XXX deallocate! */ | ||
| interp = NULL; | ||
| PyMem_RawFree(interp); |
There was a problem hiding this comment.
_PyRuntime.interpreters.head (and possibly _PyRuntime.interpreters.main) must be reverted too.
Alternately, the ID-related code could be moved to before the head/main stuff (i.e. right after the HEAD_LOCK(); line).
There was a problem hiding this comment.
I think moving the ID-related code before the HEAD_LOCK is more clear.
There was a problem hiding this comment.
Sorry for the confusion here. The ID code needs to be protected by the lock, so HEAD_LOCK() should move to right above the if (_PyRuntime.interpreters.next_id < 0) { line (i.e. move line 184 of the changed file to right before line 174). Otherwise, moving the head/main stuff down after the ID stuff looks good.
There was a problem hiding this comment.
FWIW, I don't recall why I originally put the ID-related code after the the head/main code. I vaguely remember there was a reason (and wish I had left a comment). Regardless, at this point I do not see any reason why putting the ID-related code would be a problem. :)
There was a problem hiding this comment.
Is not going to be a problem if we return in the middle of the HEAD_LOCK() - HEAD_UNLOCK() macros?
There was a problem hiding this comment.
In commit b48d5bd I changed the code to return outside the macro blocks and protect the ID stuff and HEAD stuff with the HEAD_LOCK().
|
When you're done making the requested changes, leave the comment: |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @ericsnowcurrently: please review the changes made to this pull request. |
|
LGTM Thanks for working on this! The chance of this being a problem is super remote, but I'm glad this is resolved. :) |
vstinner
left a comment
There was a problem hiding this comment.
For the NEWS entry, I would prefer to mention PyInterpreterState_New() than pystate.c.
…akage in `pystate.c`
|
Done in 9dda7fb |
| @@ -0,0 +1 @@ | |||
| Prevent a null pointer derefence and resource leakage in `PyInterpreterState_New()`. | |||
There was a problem hiding this comment.
Use double backquotes, or
:c:func:`PyInterpreterState_New`There was a problem hiding this comment.
Also, there's a typo: "derefence" should be "dereference".
| } | ||
| HEAD_UNLOCK(); | ||
|
|
||
| if (!interp){ |
There was a problem hiding this comment.
This doesn't comply with PEP 7.
…urce leakage in `pystate.c`
…age in `PyInterpreterState_New()` (pythonGH-8767) * A pointer in `PyInterpreterState_New()` could have been `NULL` when being dereferenced. * Memory was leaked in `PyInterpreterState_New()` when taking some error-handling code path. (cherry picked from commit 95d630e) Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
…age in `PyInterpreterState_New()` (GH-8767) (GH-13237) * A pointer in `PyInterpreterState_New()` could have been `NULL` when being dereferenced. * Memory was leaked in `PyInterpreterState_New()` when taking some error-handling code path. (cherry picked from commit 95d630e) Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
https://bugs.python.org/issue34408