classification
Title: Memory mismanagement with Py_tp_doc
Type: Stage:
Components: Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: georg.brandl, loewis, pitrou
Priority: release blocker Keywords: patch

Created on 2011-02-19 16:38 by loewis, last changed 2011-02-19 21:47 by georg.brandl. This issue is now closed.

Files
File name Uploaded Description Edit
tp_doc.diff loewis, 2011-02-19 16:38
Messages (12)
msg128857 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-02-19 16:38
Currently, memory management for the Py_tp_doc slot in PyType_FromSpec is ill-defined. The doc string being passed is stored in the type object as-is, but later released with PyObject_Free. To make this consistent, PyType_FromSpec should copy the string, so that it's guaranteed that relasing the memory matches allocation.

Without this patch, users will have to hold onto the type objects they get from PyType_FromSpec "forever".
msg128858 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-19 16:44
Sure it should be a blocker?
msg128859 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-02-19 16:48
If this isn't added, people using the API might see crashes. If they then work around crashes, they will see memory leaks in future releases.
msg128860 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-19 17:00
Well, isn't Py_tp_doc characteristically bound to a constant char *, so that the problem doesn't manifest itself?
msg128863 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-02-19 17:26
Sounds like a blocker to me.  Martin, will you be able to provide a patch before final?
msg128871 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-02-19 18:25
> Well, isn't Py_tp_doc characteristically bound to a constant char *

That's exactly the problem: PyType_FromSpec puts in this string literal.
When the type is later released, PyObject_Free is called in
type_dealloc, which segfaults.
msg128872 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-02-19 18:26
> Sounds like a blocker to me.  Martin, will you be able to provide a patch before final?

The attached patch works fine for me.
msg128873 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-19 18:34
> > Sounds like a blocker to me.  Martin, will you be able to provide a patch before final?
> 
> The attached patch works fine for me.

Is there some documentation somewhere for all this?
msg128879 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-02-19 19:39
>>> Sounds like a blocker to me.  Martin, will you be able to provide a patch before final?
>>
>> The attached patch works fine for me.
> 
> Is there some documentation somewhere for all this?

Can you please be more specific? What's "all this"?

That tp_doc will be copied is currently undocumented. PyType_FromSpec
is documented in the PEP.
msg128880 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-02-19 19:41
> >>> Sounds like a blocker to me.  Martin, will you be able to provide a patch before final?
> >>
> >> The attached patch works fine for me.
> > 
> > Is there some documentation somewhere for all this?
> 
> Can you please be more specific? What's "all this"?
> 
> That tp_doc will be copied is currently undocumented. PyType_FromSpec
> is documented in the PEP.

I meant all of py_tp_doc, "limited api", "restricted api" show almost
zero hits in the API docs. A PEP is nice but I'm not sure many people
spontaneously read PEPs.
msg128881 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2011-02-19 19:48
>> That tp_doc will be copied is currently undocumented. PyType_FromSpec
>> is documented in the PEP.
> 
> I meant all of py_tp_doc, "limited api", "restricted api" show almost
> zero hits in the API docs. A PEP is nice but I'm not sure many people
> spontaneously read PEPs.

No, there is no documentation in the Doc directory. I *would* expect
that people interested in using the stable ABI do find the PEP and
read it. With the exception of this issue, I consider the PEP to be
complete and correct documentation (of course, it probably contains
errors that I'm unaware of).

However, documentation for the PEP 384 changes seems to be an
unrelated issue.
msg128882 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2011-02-19 21:47
OK, committed in r88443 (with error handling made more consistent).

BTW, the return from PyType_GenericAlloc isn't NULL-checked, which looks like a potential crasher to me.  Not release-critical though.
History
Date User Action Args
2011-02-19 21:47:49georg.brandlsetstatus: open -> closed

messages: + msg128882
resolution: fixed
2011-02-19 19:48:52loewissetmessages: + msg128881
2011-02-19 19:41:28pitrousetmessages: + msg128880
2011-02-19 19:39:43loewissetmessages: + msg128879
2011-02-19 18:34:36pitrousetmessages: + msg128873
2011-02-19 18:26:18loewissetmessages: + msg128872
2011-02-19 18:25:48loewissetmessages: + msg128871
2011-02-19 17:26:35georg.brandlsetmessages: + msg128863
2011-02-19 17:00:37pitrousetmessages: + msg128860
2011-02-19 16:48:34loewissetmessages: + msg128859
2011-02-19 16:44:38pitrousetnosy: + georg.brandl, pitrou
messages: + msg128858
2011-02-19 16:38:30loewiscreate