classification
Title: "s*" argument parser marker leaks memory
Type: resource usage Stage:
Components: Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: amaury.forgeotdarc, pitrou
Priority: release blocker Keywords: needs review, patch

Created on 2008-08-24 21:02 by amaury.forgeotdarc, last changed 2008-08-29 18:42 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
argleak.patch pitrou, 2008-08-25 12:38
argleak2.patch pitrou, 2008-08-25 13:42
argleak-2.6.patch pitrou, 2008-08-25 14:19
argleak3.patch pitrou, 2008-08-25 14:31
argleak2-2.6.patch pitrou, 2008-08-25 14:39
Messages (12)
msg71870 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-24 21:02
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 issue501716)
msg71917 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-25 12:38
Here is a patch. Please review.
msg71918 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-25 12:57
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);
msg71923 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-25 13:42
Ok, here is a new patch addressing Amaury's comments.
msg71924 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-25 14:02
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]
msg71925 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-25 14:19
Here is a patch for 2.6.
msg71926 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-25 14:27
Both patches look good to me.
msg71927 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-25 14:31
Actually, here is a better patch for py3k.
msg71929 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-25 14:39
And a similarly better patch for 2.6.
msg72123 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-29 10:00
Amaury, are these patches ok to check in?
msg72126 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-08-29 11:50
Yes, let them go in!
msg72149 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-29 18:42
Committed in r66057 and r66058.
History
Date User Action Args
2008-08-29 18:42:00pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg72149
2008-08-29 11:50:52amaury.forgeotdarcsetresolution: accepted
messages: + msg72126
2008-08-29 10:00:06pitrousetmessages: + msg72123
2008-08-25 14:39:57pitrousetfiles: + argleak2-2.6.patch
messages: + msg71929
versions: + Python 2.6
2008-08-25 14:31:42pitrousetfiles: + argleak3.patch
messages: + msg71927
2008-08-25 14:27:36amaury.forgeotdarcsetmessages: + msg71926
2008-08-25 14:19:37pitrousetfiles: + argleak-2.6.patch
messages: + msg71925
2008-08-25 14:02:11pitrousetmessages: + msg71924
2008-08-25 13:42:07pitrousetfiles: + argleak2.patch
messages: + msg71923
2008-08-25 12:57:07amaury.forgeotdarcsetmessages: + msg71918
2008-08-25 12:38:59pitrousetkeywords: + patch, needs review
files: + argleak.patch
type: resource usage
messages: + msg71917
2008-08-25 10:07:17pitrousetassignee: pitrou
2008-08-25 09:32:01pitrousetnosy: + pitrou
2008-08-24 21:02:57amaury.forgeotdarccreate