Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Py_BuildValue loses reference counts on error #40503

Closed
shredwheat mannequin opened this issue Jul 3, 2004 · 8 comments
Closed

Py_BuildValue loses reference counts on error #40503

shredwheat mannequin opened this issue Jul 3, 2004 · 8 comments
Assignees
@mwhudson
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@shredwheat
Copy link
Mannequin

shredwheat mannequin commented Jul 3, 2004

BPO 984722
Nosy @mwhudson, @rhettinger
Files
  • buildvalue_patch.diff: patch
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/mwhudson'
closed_at = <Date 2004-07-14.11:28:34.000>
created_at = <Date 2004-07-03.20:19:26.000>
labels = ['interpreter-core']
title = 'Py_BuildValue loses reference counts on error'
updated_at = <Date 2004-07-14.11:28:34.000>
user = 'https://bugs.python.org/shredwheat'

    bugs.python.org fields:

    activity = <Date 2004-07-14.11:28:34.000>
actor = 'mwh'
assignee = 'mwh'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2004-07-03.20:19:26.000>
creator = 'shredwheat'
dependencies = []
files = ['1329']
hgrepos = []
issue_num = 984722
keywords = []
message_count = 8.0
messages = ['21392', '21393', '21394', '21395', '21396', '21397', '21398', '21399']
nosy_count = 3.0
nosy_names = ['mwh', 'rhettinger', 'shredwheat']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue984722'
versions = ['Python 2.4']
    @shredwheat
    Copy link
    Mannequin Author

    shredwheat mannequin commented Jul 3, 2004

    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.

    @shredwheat shredwheat mannequin closed this as completed Jul 3, 2004
    @shredwheat shredwheat mannequin assigned mwhudson Jul 3, 2004
    @shredwheat shredwheat mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jul 3, 2004
    @shredwheat shredwheat mannequin closed this as completed Jul 3, 2004
    @shredwheat shredwheat mannequin assigned mwhudson Jul 3, 2004
    @shredwheat shredwheat mannequin added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jul 3, 2004
    @mwhudson
    Copy link

    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...

    @rhettinger
    Copy link
    Contributor

    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.

    @shredwheat
    Copy link
    Mannequin Author

    shredwheat mannequin commented Jul 12, 2004

    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.

    @rhettinger
    Copy link
    Contributor

    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.

    @shredwheat
    Copy link
    Mannequin Author

    shredwheat mannequin commented Jul 13, 2004

    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)

    @mwhudson
    Copy link

    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.

    @mwhudson
    Copy link

    Logged In: YES
    user_id=6656

    Checked in as

    Python/modsupport.c revision 2.71

    Thanks!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 9, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @mwhudson mwhudson

    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @mwhudson @rhettinger