classification
Title: SEGFAULT when deleting Exception.message
Type: crash Stage: resolved
Components: Extension Modules, Interpreter Core Versions: Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, amaury.forgeotdarc, jcea, jcon, mark.dickinson, pitrou, python-dev, serhiy.storchaka, vstinner
Priority: high Keywords: patch

Created on 2012-11-08 23:05 by amaury.forgeotdarc, last changed 2013-05-04 20:26 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
py_clear.spatch vstinner, 2012-11-09 00:59
python27_pyclear.patch vstinner, 2012-11-09 00:59 review
issue16445.patch mark.dickinson, 2013-02-10 18:56 review
Messages (13)
msg175203 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-11-08 23:05
The script below segfaults cpython2.7. 
The cause is in BaseException_set_message(), which calls Py_XDECREF(self->message) and thus can call back into Python code with a dangling PyObject* pointer. Py_CLEAR should be used instead.


class Nasty(str):
    def __del__(self):
        del e.message

e = ValueError(Nasty("msg"))
e.args = ()
del e.message
del e
msg175211 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-11-09 00:59
According to Amaury an IRC, replacing "Py_XDECREF(obj->attr); obj->attr = NULL;" pattern with "Py_CLEAR(obj->attr);" should fix the issue.

Instead of opening one issue per occurence of this pattern, it is possible to write a semantic patch to do the replace on the whole CPython project using the Coccinelle tool.
http://coccinelle.lip6.fr/

See for example attached py_clear.spatch semantic patch. Install coccinelle and run "spatch -in_place -sp_file py_clear.spatch -dir .". The result is the attached patch (python2.7_pyclear.patch). The patch is huge!

 Mac/Modules/_scproxy.c                   |    2 +-
 Mac/Modules/carbonevt/_CarbonEvtmodule.c |    3 +-
 Mac/Modules/list/_Listmodule.c           |    3 +-
 Modules/_bsddb.c                         |   48 ++++---------
 Modules/_ctypes/_ctypes.c                |   18 +---
 Modules/_elementtree.c                   |    8 +-
 Modules/_hotshot.c                       |    3 +-
 Modules/_localemodule.c                  |    3 +-
 Modules/_sqlite/connection.c             |    6 +-
 Modules/_sqlite/cursor.c                 |   12 +--
 Modules/_tkinter.c                       |    6 +-
 Modules/almodule.c                       |    3 +-
 Modules/binascii.c                       |   21 ++----
 Modules/bz2module.c                      |   12 +--
 Modules/cPickle.c                        |    3 +-
 Modules/cdmodule.c                       |   18 +---
 Modules/cjkcodecs/multibytecodec.c       |    3 +-
 Modules/datetimemodule.c                 |   12 +--
 Modules/flmodule.c                       |   12 +--
 Modules/fmmodule.c                       |    3 +-
 Modules/nismodule.c                      |    3 +-
 Modules/posixmodule.c                    |   33 +++------
 Modules/pyexpat.c                        |    6 +-
 Modules/readline.c                       |    7 +-
 Modules/selectmodule.c                   |    3 +-
 Modules/signalmodule.c                   |    9 +-
 Modules/svmodule.c                       |    3 +-
 Modules/syslogmodule.c                   |    3 +-
 Modules/zlibmodule.c                     |   18 +---
 Objects/abstract.c                       |    6 +-
 Objects/classobject.c                    |   18 +---
 Objects/descrobject.c                    |    3 +-
 Objects/exceptions.c                     |    3 +-
 Objects/fileobject.c                     |   12 +--
 Objects/frameobject.c                    |    3 +-
 Objects/genobject.c                      |    3 +-
 Objects/longobject.c                     |    3 +-
 Objects/object.c                         |    6 +-
 Objects/stringobject.c                   |   12 +--
 Objects/tupleobject.c                    |    6 +-
 Objects/typeobject.c                     |   12 +--
 Objects/unicodeobject.c                  |   12 +--
 Python/Python-ast.c                      |  381 ++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------
 Python/bltinmodule.c                     |   12 +--
 Python/errors.c                          |    3 +-
 Python/import.c                          |    6 +-
 Python/marshal.c                         |   12 +--
 Python/modsupport.c                      |    3 +-
 Python/structmember.c                    |    3 +-
 Python/sysmodule.c                       |    9 +-
 RISCOS/Modules/drawfmodule.c             |    2 +-
 RISCOS/Modules/swimodule.c               |    2 +-
 52 files changed, 275 insertions(+), 541 deletions(-)
msg175212 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-11-09 01:01
The "spatch" doesn't match the following macro:

Modules/_testcapimodule.c:#define UNBIND(X)  Py_DECREF(X); (X) = NULL

--


Python/Python-ast.c is autogenerated from Parser/asdl_c.py, the .py file should be fixed instead.
msg175213 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-11-09 01:04
python27_pyclear.patch does fix the use case described in msg175203, but it doesn't fix #16447.
msg175216 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-11-09 01:18
py_clear.spatch	replaces:
  Py_DECREF(obj->attr); obj->attr = NULL;
but also:
  Py_DECREF(obj); obj = NULL;

If the second pattern is not dangerous, py_clear.spatch can be modified to only match the first pattern: see py_decref_replace.spatch of issue #16447 for an example.
msg175224 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-11-09 11:40
The Coccinelle looks as an amazing tool!

I propose split a patch on several patches (autogenerated code, attributes clearing, other pointer expressions (as *exceptionObject or unicode_latin1[i]) clearing, and local pointers clearing at the end) and commit they separately.
msg176910 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-04 12:58
Victor, I will be glad to review any related patches, but please split the patch on several patches, from most undoubted to more sophisticated patterns.
msg181839 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-02-10 18:52
Can I suggest fixing this particular issue with a dedicated patch, and opening another issue to consider the large automated replacements that Victor's proposing?
msg181840 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2013-02-10 18:56
Here's a simple patch (against 2.7) for this particular issue.
msg181841 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-10 19:07
> Here's a simple patch (against 2.7) for this particular issue.

LGTM.
msg183368 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-03-03 11:13
New changeset 0e41c4466d58 by Mark Dickinson in branch '2.7':
Issue #16445: Fix potential segmentation fault when deleting an exception message.
http://hg.python.org/cpython/rev/0e41c4466d58
msg187262 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2013-04-18 15:51
It looks as if this issue can be closed as a fix has been committed.  However a new issue will be needed if the advice offered in msg181839 is followed.
msg188405 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-05-04 20:26
This looks like it is fixed by Mark's commit. Other proposals should go into separate issues.
History
Date User Action Args
2013-05-04 20:26:47pitrousetstatus: open -> closed

versions: - Python 3.2, Python 3.3, Python 3.4
nosy: + pitrou

messages: + msg188405
resolution: fixed
stage: needs patch -> resolved
2013-04-18 15:51:10BreamoreBoysetnosy: + BreamoreBoy
messages: + msg187262
2013-03-03 11:13:48python-devsetnosy: + python-dev
messages: + msg183368
2013-02-10 19:07:33serhiy.storchakasetmessages: + msg181841
2013-02-10 18:56:51mark.dickinsonsetfiles: + issue16445.patch

messages: + msg181840
2013-02-10 18:52:50mark.dickinsonsetmessages: + msg181839
2013-01-27 12:04:01serhiy.storchakasetpriority: normal -> high
2013-01-11 13:33:26mark.dickinsonsetnosy: + mark.dickinson
2013-01-10 04:43:58jconsetnosy: + jcon
2012-12-04 12:58:16serhiy.storchakasetmessages: + msg176910
2012-11-10 01:42:25jceasetnosy: + jcea
2012-11-09 11:40:44serhiy.storchakasetversions: + Python 2.7, Python 3.2, Python 3.3, Python 3.4
nosy: + serhiy.storchaka

messages: + msg175224

components: + Extension Modules, Interpreter Core
stage: needs patch
2012-11-09 01:18:29vstinnersetmessages: + msg175216
2012-11-09 01:04:14vstinnersetmessages: + msg175213
2012-11-09 01:01:45vstinnersetmessages: + msg175212
2012-11-09 00:59:43vstinnersetfiles: + python27_pyclear.patch
keywords: + patch
2012-11-09 00:59:14vstinnersetfiles: + py_clear.spatch

messages: + msg175211
2012-11-08 23:42:48vstinnersetnosy: + vstinner
2012-11-08 23:05:10amaury.forgeotdarccreate