Issue18287
Created on 2013-06-23 15:12 by nkoep, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| patch.default | nkoep, 2013-06-28 11:48 | review | ||
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 552 | closed | dstufft, 2017-03-31 16:36 | |
| Messages (8) | |||
|---|---|---|---|
| msg191705 - (view) | Author: Niklas Koep (nkoep) | Date: 2013-06-23 15:12 | |
I noticed that defining a new type where the tp_name field is NULL causes segfaults, for instance, when calling pydoc on the extension module. This particular segfault traces back to type_module() in Objects/typeobject.c where tp_name is passed to strrchr(). Raising an appropriate exception from PyType_Ready() seems sensible to avoid this kind of issue. The field is also used in two calls to PyErr_Format() in PyType_Ready() itself where it'll cause segfaults if not handled properly. While we're on the subject, pydoc doesn't list the type documentation if the tp_name string does not have a dot in it. I didn't research this any further as omitting dots seems to be valid according to the docs. However, at the very least it seems like this side effect should be mentioned in the documentation to avoid confusion when someone omits/forgets the package.<subpackage>.module.type hierarchy in the field definition. I attached a tiny patch which just jumps to PyType_Ready()'s error label if the field is NULL. I also added a comment about pydoc in the two places of the documentation I could think of where tp_name is mentioned. |
|||
| msg191986 - (view) | Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * | Date: 2013-06-28 10:05 | |
The patch is not complete: PyType_Ready() returns -1 but no no exception is set. |
|||
| msg191990 - (view) | Author: Niklas Koep (nkoep) | Date: 2013-06-28 11:48 | |
Oh, you're right, of course. I completely forgot that any other case which jumps to the error label assumes an appropriate exception has already been set. I attached a new patch which raises a TypeError. Is there a better way to identify the type which is affected by this exception? Since we're complaining about the missing tp_name field we obviously can't supply the name in the exception. |
|||
| msg191991 - (view) | Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * | Date: 2013-06-28 12:23 | |
Since this error can occur only during the development of a C extension, I would not worry too much. The traceback will already indicate the imported module, this is much better than a segfault later in pydoc. |
|||
| msg258336 - (view) | Author: Rose Ames (superluser) * | Date: 2016-01-15 22:43 | |
There's still no check on tp_name. The patch looks reasonable, applies cleanly, compiles, and doesn't break any tests - suggest it be merged. |
|||
| msg277882 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2016-10-02 10:31 | |
The patch LGTM, but I think SystemError is more appropriate. |
|||
| msg278266 - (view) | Author: Roundup Robot (python-dev) | Date: 2016-10-07 20:27 | |
New changeset 5c459b0f2b75 by Serhiy Storchaka in branch '3.5': Issue #18287: PyType_Ready() now checks that tp_name is not NULL. https://hg.python.org/cpython/rev/5c459b0f2b75 New changeset ba76dd106e66 by Serhiy Storchaka in branch '2.7': Issue #18287: PyType_Ready() now checks that tp_name is not NULL. https://hg.python.org/cpython/rev/ba76dd106e66 New changeset 0b726193ec3c by Serhiy Storchaka in branch '3.6': Issue #18287: PyType_Ready() now checks that tp_name is not NULL. https://hg.python.org/cpython/rev/0b726193ec3c New changeset a60d0e80cc1d by Serhiy Storchaka in branch 'default': Issue #18287: PyType_Ready() now checks that tp_name is not NULL. https://hg.python.org/cpython/rev/a60d0e80cc1d |
|||
| msg278267 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2016-10-07 20:28 | |
Thank you Niklas for your report and patch. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2017-03-31 16:36:27 | dstufft | set | pull_requests: + pull_request1009 |
| 2016-10-07 20:28:15 | serhiy.storchaka | set | status: open -> closed resolution: fixed messages: + msg278267 stage: patch review -> resolved |
| 2016-10-07 20:27:12 | python-dev | set | nosy:
+ python-dev messages: + msg278266 |
| 2016-10-02 10:31:01 | serhiy.storchaka | set | assignee: serhiy.storchaka messages: + msg277882 |
| 2016-10-02 10:25:26 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka versions: + Python 3.7, - Python 3.4 |
| 2016-01-15 22:43:53 | superluser | set | nosy:
+ superluser messages: + msg258336 |
| 2015-11-14 23:47:13 | serhiy.storchaka | set | versions: + Python 3.5, Python 3.6, - Python 3.3 |
| 2013-07-19 16:57:35 | nkoep | set | files: - patch |
| 2013-06-28 12:23:47 | amaury.forgeotdarc | set | messages: + msg191991 |
| 2013-06-28 11:48:33 | nkoep | set | files:
+ patch.default messages: + msg191990 |
| 2013-06-28 10:05:46 | amaury.forgeotdarc | set | messages: + msg191986 |
| 2013-06-28 09:56:50 | pitrou | set | nosy:
+ amaury.forgeotdarc, benjamin.peterson stage: patch review versions: + Python 2.7, Python 3.3, Python 3.4, - Python 3.5 |
| 2013-06-23 15:12:52 | nkoep | create | |