This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Py_BuildValue loses reference counts on error
Type: Stage:
Components: Interpreter Core Versions: Python 2.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mwh Nosy List: mwh, rhettinger, shredwheat
Priority: normal Keywords:

Created on 2004-07-03 20:19 by shredwheat, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
buildvalue_patch.diff shredwheat, 2004-07-13 20:59 patch
Messages (8)
msg21392 - (view) Author: Pete Shinners (shredwheat) Date: 2004-07-03 20:19
Py_BuildValue has the convenient "N" type argument to
take a PyObject* without a reference count. To the
programmer this means your are transferring the object
ownership to the Py_BuildValue function (and the tuple
it creates).

If Py_BuildValue encounters an error processing an
argument it aborts and returns NULL. But the remaining
arguments are ignored. Therefore objects are leaked
from a leftover reference count.

From looking at the code in Python/modsupport.c it
looks like a reasonable solution would be to insert a
Py_None into the tuple/list/dict being created and
internally set some sort of error flag. When the
function is returning it would check the error flag and
if set, Py_DECREF the created object and return NULL.

At first this may seem like a lot of work to do when we
already know the function will fail. But it is no more
work than we would be doing when the function succeeeds.
msg21393 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2004-07-12 12:30
Logged In: YES 
user_id=6656

Um.  Any particular reason for assigning to me?  I could probably 
review and apply any patch that appears, but I'm unlikely to get to 
creating one myself...
msg21394 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2004-07-12 13:36
Logged In: YES 
user_id=80475

I thought you would have an interest and be in a good
position to know whether and how this should be fixed.  Feel
free to unassign.
msg21395 - (view) Author: Pete Shinners (shredwheat) Date: 2004-07-12 14:29
Logged In: YES 
user_id=1076442

I will attempt a patch, this does not look difficult. I am
currently trying to get straight CVS Python to run the test
suite, but having difficulty. I will have a patch ready or
more comments otherwise later this week.
msg21396 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2004-07-12 17:59
Logged In: YES 
user_id=80475

Thanks, that will be nice.

Do make a special effort to make sure this has zero
performance impact on the common case.  No sense in mucking
up the works to satisfy a corner case that rarely impacts
real apps.
msg21397 - (view) Author: Pete Shinners (shredwheat) Date: 2004-07-13 20:59
Logged In: YES 
user_id=1076442

Here is patch, please review and let me know if further
revisions are necessary. Not sure if the comment line in
each error case is helpful or a distraction.

Performance difference is negligable for the common
'success' case. If multiple arguments raise exceptions it is
undefined which one is current when the function returns. A
NULL will still be returned if there are any exceptions.

(my first Python patch, exciting)
msg21398 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2004-07-14 11:11
Logged In: YES 
user_id=6656

Looks OK, I'll probably check it in (with slightly rearranged 
comments) after make test finishes.

Is this testable?  Maybe in testcapimodule. Maybe it's not worth it.
msg21399 - (view) Author: Michael Hudson (mwh) (Python committer) Date: 2004-07-14 11:28
Logged In: YES 
user_id=6656

Checked in as 

Python/modsupport.c revision 2.71

Thanks!
History
Date User Action Args
2022-04-11 14:56:05adminsetgithub: 40503
2004-07-03 20:19:26shredwheatcreate