This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Fix or drop Lib/test/crashers/borrowed_ref_1.py
Type: crash Stage:
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: arigo, benjamin.peterson, python-dev, vstinner
Priority: normal Keywords:

Created on 2012-03-09 00:05 by vstinner, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (6)
msg155190 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-03-09 00:05
Lib/test/crashers/borrowed_ref_1.py contains the docstring:
"_PyType_Lookup() returns a borrowed reference.
This attacks the call in dictobject.c."
The file was added by Armin in 2006 by the changeset 4dd1a9383e47.

I just fixed #14211 which was a similar bug (Lib/test/crashers/borrowed_ref_2.py), but I'm unable to reproduce this one.

Extract of dict_subscript():

            missing = _PyObject_LookupSpecial((PyObject *)mp, &PyId___missing__);
            if (missing != NULL) {
                res = PyObject_CallFunctionObjArgs(missing,
                                                   key, NULL);
                Py_DECREF(missing);
                return res;
            }

I fail to see how missing can be destroyed before being used. Is it a problem that missing is destroyed while we are calling it?

How can I trigger the crash in gdb?
msg155191 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2012-03-09 00:14
Crashes for me on Python 2.5 and 2.6, but no longer on Python 2.7.  The problem may have been fixed in the meantime.
msg155193 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-03-09 00:20
Oh, it looks like the following commit fixed the issue without touching to the crasher test: ba6376dff6c4. Armin, Benjamin: can you confirm?
msg155228 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2012-03-09 12:59
ba6376dff6c4 changed dict_subscript(): _PyType_Lookup() is replaced by _PyObject_LookupSpecial(). 7b4b921f3335 fixed a reference introduced in this change.

_PyObject_Lookup() returns a borrowed reference, whereas _PyObject_LookupSpecial() increments the reference to the attribute. The issue was on borrowed reference, so the bug is already fixed.
msg155229 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-03-09 13:00
New changeset 850d7f6af1b0 by Victor Stinner in branch 'default':
Issue #14231: Lib/test/crashers/borrowed_ref_1.py was fixed by ba6376dff6c4.
http://hg.python.org/cpython/rev/850d7f6af1b0
msg155238 - (view) Author: Armin Rigo (arigo) * (Python committer) Date: 2012-03-09 16:07
I will attempt a last time to mention that the docstrings in borrowed_ref_*.py used to say they were *examples*.

That means: (1) find any internal or external C function that returns a borrowed reference; (2) find all callers and write down all the places that don't immediately either Py_INCREF() the returned value or immediately forget about it; (3) for each such place, either come up painfully with a complicated explanation of why it's safe in all possible cases, or in doubt, just fix it by adding Py_INCREF()/Py_DECREF().

What I did in writing these two borrowed_ref_*.py was to do instead (3') spend a few hours figuring out how to exploit the issue until we get a segfault.  I did it for two examples, but what I'm definitely not going to do is spend N times a few hours for a large number N.  If python-dev people just fix the two examples, remove the crashers, and just forget about the issue, then well, the point is missed, but I'm not going to fight it.
History
Date User Action Args
2022-04-11 14:57:27adminsetgithub: 58439
2012-03-09 16:07:37arigosetmessages: + msg155238
2012-03-09 13:00:45vstinnersetstatus: open -> closed
resolution: fixed
2012-03-09 13:00:13python-devsetnosy: + python-dev
messages: + msg155229
2012-03-09 12:59:54vstinnersetmessages: + msg155228
2012-03-09 00:20:48vstinnersetnosy: + benjamin.peterson
messages: + msg155193
2012-03-09 00:14:01arigosetmessages: + msg155191
2012-03-09 00:05:36vstinnercreate