Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEGFAULT when setting type.__name__ #60651

Closed
amauryfa opened this issue Nov 9, 2012 · 11 comments
Closed

SEGFAULT when setting type.__name__ #60651

amauryfa opened this issue Nov 9, 2012 · 11 comments
Assignees
Labels
extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@amauryfa
Copy link
Member

amauryfa commented Nov 9, 2012

BPO 16447
Nosy @jcea, @amauryfa, @mdickinson, @vstinner, @asvetlov, @meadori, @serhiy-storchaka
Files
  • py_decref_replace.spatch
  • python27_decref_replace.patch
  • issue16447_27.patch
  • issue16447_32.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/mdickinson'
    closed_at = <Date 2013-04-13.14:31:31.281>
    created_at = <Date 2012-11-09.00:09:20.370>
    labels = ['extension-modules', 'interpreter-core', 'type-crash']
    title = 'SEGFAULT when setting type.__name__'
    updated_at = <Date 2013-04-13.14:31:37.512>
    user = 'https://github.com/amauryfa'

    bugs.python.org fields:

    activity = <Date 2013-04-13.14:31:37.512>
    actor = 'mark.dickinson'
    assignee = 'mark.dickinson'
    closed = True
    closed_date = <Date 2013-04-13.14:31:31.281>
    closer = 'mark.dickinson'
    components = ['Extension Modules', 'Interpreter Core']
    creation = <Date 2012-11-09.00:09:20.370>
    creator = 'amaury.forgeotdarc'
    dependencies = []
    files = ['27931', '27932', '29298', '29299']
    hgrepos = []
    issue_num = 16447
    keywords = ['patch']
    message_count = 11.0
    messages = ['175210', '175214', '175215', '175225', '175228', '175258', '183386', '183387', '186718', '186719', '186720']
    nosy_count = 9.0
    nosy_names = ['jcea', 'amaury.forgeotdarc', 'mark.dickinson', 'vstinner', 'Arfrever', 'asvetlov', 'meador.inge', 'python-dev', 'serhiy.storchaka']
    pr_nums = []
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue16447'
    versions = ['Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4']

    @amauryfa
    Copy link
    Member Author

    amauryfa commented Nov 9, 2012

    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"

    @amauryfa amauryfa added the type-crash A hard crash of the interpreter, possibly with a core dump label Nov 9, 2012
    @vstinner
    Copy link
    Member

    vstinner commented Nov 9, 2012

    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 bpo-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(-)

    @vstinner
    Copy link
    Member

    vstinner commented Nov 9, 2012

    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); }".

    @serhiy-storchaka
    Copy link
    Member

    Yes, the macro appropriate here.

    In Modules/zlibmodule.c this patterns should be fixed by patch for bpo-16350.

    @serhiy-storchaka serhiy-storchaka added extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Nov 9, 2012
    @amauryfa
    Copy link
    Member Author

    amauryfa commented Nov 9, 2012

    • 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);
            }

    @jcea
    Copy link
    Member

    jcea commented Nov 10, 2012

    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?.

    @mdickinson
    Copy link
    Member

    Patch for the immediate issue, for Python 2.7. The Py_DECREF is delayed until after setting *both* ht_name and tp_name.

    @mdickinson
    Copy link
    Member

    And the corresponding patch against 3.2 (applies cleanly to 3.3 and default, modulo Misc/NEWS fixes).

    @mdickinson mdickinson self-assigned this Mar 3, 2013
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 13, 2013

    New changeset d5e5017309b1 by Mark Dickinson in branch '2.7':
    Issue bpo-16447: Fix potential segfault when setting __name__ on a class.
    http://hg.python.org/cpython/rev/d5e5017309b1

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 13, 2013

    New changeset e6d1328412c8 by Mark Dickinson in branch '3.3':
    Issue bpo-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 bpo-16447: Merge fix from 3.3.
    http://hg.python.org/cpython/rev/c8d771f10022

    @mdickinson
    Copy link
    Member

    Fixed.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants