Repository navigation
BUG: Refactor error handling in array_setstate to prevent typecode leak - #31701
Conversation
There was a problem hiding this comment.
This is a fairly complicated function so it isn't too easy to follow the flow and all the ownership. But I find it easier to reason about if you have fewer exit points and fewer paths through.
That typically means that an owned reference should either be NULL or set. References shouldn't change between being owned or unowned (like rawdata does).
So you'd have something like
PyObject *result = NULL, *typecode = NULL, *rawdata = NULL;
// ... code goes here ...
result = final_thing_that_creates_the_result();
end:
Py_XDECREF(typecode); // either NULL, or owned, XDECREF is happy both ways
Py_XDECREF(rawdata);
return result; // either NULL or set...
and then you have one exit point and everything goes through that. Every return should go through the end (except possibly right at the start of the function before anything is owned).
This probably requires taking ownership of both typecode and rawdata with a Py_INCREF immediately after PyArg_ParseTuple.
There was a problem hiding this comment.
This seems like it might still be leaking typecode
|
I agree @da-woods; the current From what I understand, the goal is to keep local references consistent, either owned or I'll take a look at restructuring the ownership and cleanup logic along those lines. |
da-woods
left a comment
da-woods
left a comment
There was a problem hiding this comment.
A few small comments (only one of which is a real error I think) but to me that looks much more likely to work right.
| * Direct pointer path: PyArray_SetBaseObject steals the rawdata | ||
| * reference. NULL rawdata afterward so Py_XDECREF at the end | ||
| * label does not double-decref a stolen ref. | ||
| */ | ||
| fa->mem_handler = NULL; | ||
| fa->data = datastr; | ||
| if (PyArray_SetBaseObject(self, rawdata) < 0) { |
There was a problem hiding this comment.
Most reference steeling functions (including PyArray_SetBaseObject it looks like) steal a reference on both success and failure to be consistent
There was a problem hiding this comment.
I’ll recheck the ownership contract here... and adjust the cleanup accordingly
| Py_DECREF(fa->descr); | ||
| fa->descr = swapped; |
There was a problem hiding this comment.
You might be better doing Py_SETREF(fa->descr, swapped) (because can in principle cause arbitrary code to execute so you're leaving it in an potentially invalid state while that happens).
I don't think it can actually happen here, but I think it's a good idea anyway
There was a problem hiding this comment.
Okay... I’ll switch this assignment.
| Py_DECREF(fa->descr); | ||
| fa->descr = swapped; |
|
I looked at how I changed the code to transfer ownership before calling Could you take another look, @da-woods? Does this match what you had in mind? |
Yes that looks right to me. |
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Thanks @whyvineet and especially also @da-woods for reviewing. This is a bit of a dark corner, the fix is I think in a sense meaningless (you would need a bad pickle to hit it and at that point anything is possible).
(We use finish: more often than end: but I don't feel like style nitpicking, let's get it in.)
charris
changed the title
FIX: Refactor error handling in array_setstate to prevent typecode leak (#31701)
PR summary
This fixes the reference-count imbalance reported in #31678
AI Disclosure
No AI tools used