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

"s*" argument parser marker leaks memory #47918

Closed
amauryfa opened this issue Aug 24, 2008 · 12 comments
Closed

"s*" argument parser marker leaks memory #47918

amauryfa opened this issue Aug 24, 2008 · 12 comments
Assignees
@pitrou
Labels
performance Performance or resource usage release-blocker

Comments

@amauryfa
Copy link
Member

BPO 3668
Nosy @amauryfa, @pitrou
Files
  • argleak.patch
  • argleak2.patch
  • argleak-2.6.patch
  • argleak3.patch
  • argleak2-2.6.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/pitrou'
closed_at = <Date 2008-08-29.18:42:00.425>
created_at = <Date 2008-08-24.21:02:57.594>
labels = ['release-blocker', 'performance']
title = '"s*" argument parser marker leaks memory'
updated_at = <Date 2008-08-29.18:42:00.424>
user = 'https://github.com/amauryfa'

    bugs.python.org fields:

    activity = <Date 2008-08-29.18:42:00.424>
actor = 'pitrou'
assignee = 'pitrou'
closed = True
closed_date = <Date 2008-08-29.18:42:00.425>
closer = 'pitrou'
components = []
creation = <Date 2008-08-24.21:02:57.594>
creator = 'amaury.forgeotdarc'
dependencies = []
files = ['11243', '11244', '11245', '11246', '11248']
hgrepos = []
issue_num = 3668
keywords = ['patch', 'needs review']
message_count = 12.0
messages = ['71870', '71917', '71918', '71923', '71924', '71925', '71926', '71927', '71929', '72123', '72126', '72149']
nosy_count = 2.0
nosy_names = ['amaury.forgeotdarc', 'pitrou']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue3668'
versions = ['Python 2.6', 'Python 3.0']
    @amauryfa
    Copy link
    Member Author

    When PyArg_ParseTuple correctly parses a s* format, but raises an
    exception afterwards (for a subsequent parameter), the user code will
    not call PyBuffer_Release() and memory will leak.
    Seen by "regrtest -R:: test_binascii"

    For example:
>>> binascii.a2b_qp("", **{1:1})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: keywords must be strings
[42278 refs]
>>> binascii.a2b_qp("", **{1:1})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: keywords must be strings
[42279 refs]
>>> binascii.a2b_qp("", **{1:1})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: keywords must be strings
[42280 refs]

    The same pattern was correctly handled by the "et#" type (where the user
    has to call PyMem_Free) with the help of a cleanup list (see the
    addcleanup() function in getargs.c). (See bpo-501716)

    @pitrou pitrou self-assigned this Aug 25, 2008
    @pitrou
    Copy link
    Member

    pitrou commented Aug 25, 2008

    Here is a patch. Please review.

    @pitrou pitrou added the performance Performance or resource usage label Aug 25, 2008
    @amauryfa
    Copy link
    Member Author

    This patch elegantly reuses the existing cleanup list.

    Only two remarks:

    • there are a few tabs/spaces inconsistencies.
    • I would make the cleanup_ptr explicit:
      intead of
      addcleanup(*buffer, freelist, NULL);
      I'd prefer
      addcleanup(*buffer, freelist, cleanup_ptr);

    @pitrou
    Copy link
    Member

    pitrou commented Aug 25, 2008

    Ok, here is a new patch addressing Amaury's comments.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 25, 2008

    By the way, this bug affects 2.6 as well, although not in binascii since
    it has not been converted to use "s*":

    >>> codecs.latin_1_decode(b"", 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: latin_1_decode() argument 2 must be string or None, not int
[57425 refs]
>>> codecs.latin_1_decode(b"", 0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: latin_1_decode() argument 2 must be string or None, not int
[57426 refs]

    @pitrou
    Copy link
    Member

    pitrou commented Aug 25, 2008

    Here is a patch for 2.6.

    @amauryfa
    Copy link
    Member Author

    Both patches look good to me.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 25, 2008

    Actually, here is a better patch for py3k.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 25, 2008

    And a similarly better patch for 2.6.

    @pitrou
    Copy link
    Member

    pitrou commented Aug 29, 2008

    Amaury, are these patches ok to check in?

    @amauryfa
    Copy link
    Member Author

    Yes, let them go in!

    @pitrou
    Copy link
    Member

    pitrou commented Aug 29, 2008

    Committed in r66057 and r66058.

    @pitrou pitrou closed this as completed Aug 29, 2008
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @pitrou pitrou

    Labels
    performance Performance or resource usage release-blocker
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @amauryfa @pitrou