classification
Title: SEGFAULT when setting type.__name__
Type: crash Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 3.4, Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: mark.dickinson Nosy List: Arfrever, amaury.forgeotdarc, asvetlov, haypo, jcea, mark.dickinson, meador.inge, python-dev, serhiy.storchaka
Priority: high Keywords: patch

Created on 2012-11-09 00:09 by amaury.forgeotdarc, last changed 2013-04-13 14:31 by mark.dickinson. This issue is now closed.

Files
File name Uploaded Description Edit
py_decref_replace.spatch haypo, 2012-11-09 01:14
python27_decref_replace.patch haypo, 2012-11-09 01:15 review
issue16447_27.patch mark.dickinson, 2013-03-03 16:38 review
issue16447_32.patch mark.dickinson, 2013-03-03 16:48 review
Messages (11)
msg175210 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-11-09 00:09
Following script crashes all versions of Python.  Cause is the "Py_DECREF(et->ht_name)" in type_set_name().

class Nasty(str):
    def __del__(self):
        C.__name__ = "other"
class C(object):
    pass
C.__name__ = Nasty("abc")
C.__name__ = "normal"
msg175214 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-11-09 01:14
It looks like the bug is the pattern "Py_DECREF(obj->attr); obj->attr = new_value;". Replacing it with "{ PyObject *tmp = obj->attr; obj->attr = new_value; Py_DECREF(tmp); }" does fix this specific issue.

We can use the coccinelle tool to replace all such patterns in the whole CPython code base using attached py_decref_replace.spatch "semantic patch". See also issue #16445, I proposed a similar idea (and another semantic patch).

Attached python27_decref_replace.patch patch is the result of the command "spatch -in_place -sp_file py_decref_replace.spatch -dir .".

The patch is quite huge, I didn't read it yet :-)


 Mac/Modules/carbonevt/_CarbonEvtmodule.c |   7 +++++--
 Mac/Modules/list/_Listmodule.c           |   7 +++++--
 Modules/_bsddb.c                         |  42 ++++++++++++++++++++++++++++++------------
 Modules/_csv.c                           |   7 +++++--
 Modules/_ctypes/_ctypes.c                |  98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------------------
 Modules/_curses_panel.c                  |   7 +++++--
 Modules/_elementtree.c                   |  81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------
 Modules/_json.c                          |   7 +++++--
 Modules/_sqlite/connection.c             |  28 ++++++++++++++++++++--------
 Modules/_sqlite/cursor.c                 |  42 ++++++++++++++++++++++++++++++------------
 Modules/bz2module.c                      |   9 +++++----
 Modules/cPickle.c                        |  36 +++++++++++++++++++++++++++---------
 Modules/flmodule.c                       |  28 ++++++++++++++++++++--------
 Modules/itertoolsmodule.c                |   7 +++++--
 Modules/selectmodule.c                   |   7 +++++--
 Modules/signalmodule.c                   |   7 +++++--
 Modules/svmodule.c                       |   7 +++++--
 Modules/zlibmodule.c                     |  23 +++++++++++++++--------
 Objects/descrobject.c                    |   7 +++++--
 Objects/exceptions.c                     |  21 +++++++++++++++------
 Objects/fileobject.c                     |  14 ++++++++++----
 Objects/funcobject.c                     |   7 +++++--
 Objects/typeobject.c                     |  21 +++++++++++++++------
 Python/ceval.c                           |   7 +++++--
 Python/sysmodule.c                       |   7 +++++--
 25 files changed, 382 insertions(+), 152 deletions(-)
msg175215 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-11-09 01:16
We should maybe use a macro (ex: Py_DECREC_REPLACE) instead of copying the pattern "{ PyObject *tmp = obj->attr; obj->attr = new_value; Py_DECREF(tmp); }".
msg175225 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-09 11:49
Yes, the macro appropriate here.

In Modules/zlibmodule.c this patterns should be fixed by patch for issue16350.
msg175228 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-11-09 12:29
- For the replacement with NULL, Py_CLEAR() should be used instead.

- We should use a macro (Py_REF_ASSIGN?) for the replacement case.

- Careful, in Modules/_json.c the code is wrong because tmp is already used::

        PyObject *tmp = PyUnicode_AsEncodedString(...);
        {
            PyObject *tmp = s->encoding;
            s->encoding = tmp;
            Py_DECREF(tmp);
        }
msg175258 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-11-10 02:38
Yes, we should add a "Py_REPLACE()" macro. Sure. +1 to that.

With this issue in mind, I wonder if there is any situation where "Py_DECREF/Py_XDECREF" must be used that can not be replace with "Py_CLEAR/Py_REPLACE".

Is there any code that breaks if we replace "Py_XDECREF()" by "Py_CLEAR()"?. Could be possible even to replace Py_DECREF definition?.
msg183386 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-03-03 16:38
Patch for the immediate issue, for Python 2.7.  The Py_DECREF is delayed until after setting *both* ht_name and tp_name.
msg183387 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-03-03 16:48
And the corresponding patch against 3.2 (applies cleanly to 3.3 and default, modulo Misc/NEWS fixes).
msg186718 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-13 14:19
New changeset d5e5017309b1 by Mark Dickinson in branch '2.7':
Issue #16447: Fix potential segfault when setting __name__ on a class.
http://hg.python.org/cpython/rev/d5e5017309b1
msg186719 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-13 14:30
New changeset e6d1328412c8 by Mark Dickinson in branch '3.3':
Issue #16447: Fix potential segfault when setting __name__ on a class.
http://hg.python.org/cpython/rev/e6d1328412c8

New changeset c8d771f10022 by Mark Dickinson in branch 'default':
Issue #16447: Merge fix from 3.3.
http://hg.python.org/cpython/rev/c8d771f10022
msg186720 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-04-13 14:31
Fixed.
History
Date User Action Args
2013-04-13 14:31:37mark.dickinsonsetstage: commit review -> resolved
2013-04-13 14:31:31mark.dickinsonsetstatus: open -> closed
2013-04-13 14:31:25mark.dickinsonsetresolution: fixed
messages: + msg186720
2013-04-13 14:30:35python-devsetmessages: + msg186719
2013-04-13 14:19:26python-devsetnosy: + python-dev
messages: + msg186718
2013-03-03 18:56:11mark.dickinsonsetassignee: mark.dickinson
2013-03-03 18:56:02mark.dickinsonsetstage: needs patch -> commit review
2013-03-03 16:48:14mark.dickinsonsetfiles: + issue16447_32.patch

messages: + msg183387
2013-03-03 16:38:23mark.dickinsonsetfiles: + issue16447_27.patch

messages: + msg183386
2013-02-10 18:51:25mark.dickinsonsetnosy: + mark.dickinson
2013-01-27 12:26:23serhiy.storchakasetpriority: normal -> high
2012-12-14 08:29:45Arfreversetnosy: + Arfrever
2012-12-11 06:32:34meador.ingesetnosy: + meador.inge
2012-11-15 15:53:42asvetlovsetnosy: + asvetlov
2012-11-10 02:38:51jceasetmessages: + msg175258
2012-11-10 01:41:07jceasetnosy: + jcea
2012-11-09 12:29:53amaury.forgeotdarcsetmessages: + msg175228
2012-11-09 11:49:26serhiy.storchakasetstage: needs patch
components: + Extension Modules, Interpreter Core
versions: + Python 2.7, Python 3.2, Python 3.3, Python 3.4
2012-11-09 11:49:03serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg175225
2012-11-09 01:16:48hayposetmessages: + msg175215
2012-11-09 01:15:08hayposetfiles: + python27_decref_replace.patch
keywords: + patch
2012-11-09 01:14:50hayposetfiles: + py_decref_replace.spatch
nosy: + haypo
messages: + msg175214

2012-11-09 00:09:20amaury.forgeotdarccreate