bpo-25750: fix refcounts in type_getattro() #6118

jdemeyer · 2018-03-14T20:10:29Z

When calling tp_descr_get(attr, obj, type), make sure that we own a reference to attr. In the existing code, we only have a borrowed reference.

This bug goes back to at least Python 2.7, so ideally it should be backported to all previous versions.

https://bugs.python.org/issue25750

Downstream: https://trac.sagemath.org/ticket/19633

the-knights-who-say-ni · 2018-03-14T20:10:31Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

berkerpeksag

In https://bugs.python.org/issue25750#msg255566 you said:

My patch does fix the crash in Lib/test/crashers/borrowed_ref_2.py on Python 2.7.10.

Is it possible to convert https://github.com/python/cpython/blob/2.7/Lib/test/crashers/borrowed_ref_2.py to a simple test case?

Thanks!

jdemeyer · 2018-03-24T20:54:52Z

Is it possible to convert https://github.com/python/cpython/blob/2.7/Lib/test/crashers/borrowed_ref_2.py to a simple test case?

I can try, but I never managed to get the vanilla Python testsuite to pass on my system, so it's hard to test it myself.

berkerpeksag · 2018-03-24T23:14:08Z

We can use Travis to test it :)

By the way, the test suite should pass on most systems. I suggest reporting test failures on your system on bugs.p.o.

jdemeyer · 2018-04-02T16:50:50Z

Is it possible to convert https://github.com/python/cpython/blob/2.7/Lib/test/crashers/borrowed_ref_2.py to a simple test case?

That example seems to crash only on Python 2, not Python 3. In fact, I do not know how to reproduce the crash on Python 3.

jdemeyer · 2018-04-20T14:04:02Z

I added a testcase. This was a lot harder to reproduce on Python 3.8 than Python 2.7, probably due to the changes regarding calling functions.

jdemeyer · 2018-04-20T14:09:06Z

So, in some sense, the backport to Python 2.7 is more important than the fix to Python 3.8. The underlying issue is the same, it's just harder to accidentally hit it because of unrelated changes.

jdemeyer · 2018-07-05T11:33:15Z

@berkerpeksag Can you please finish the review of this PR?

berkerpeksag · 2018-07-05T12:14:45Z

Lib/test/test_descr.py

+        try:
+            from _testcapi import bad_get
+        except ImportError:
+            return


We can just skip the test with an explanation instead of returning None here.

berkerpeksag · 2018-07-05T12:17:28Z

Misc/NEWS.d/next/Core and Builtins/2018-03-14-21-42-17.bpo-25750.lxgkQz.rst

@@ -0,0 +1,2 @@
+Fix rare Python crash due to bad refcounting in ``type_getattro()`` if a
+descriptor deletes itself from the class.


Please add "Patch by Jeroen Demeyer."

berkerpeksag · 2018-07-05T12:23:41Z

Modules/_testcapimodule.c

@@ -4776,6 +4798,7 @@ static PyMethodDef TestMethods[] = {
    {"get_mapping_items", get_mapping_items, METH_O},
    {"test_pythread_tss_key_state", test_pythread_tss_key_state, METH_VARARGS},
    {"hamt", new_hamt, METH_NOARGS},
+    {"bad_get", bad_get, METH_FASTCALL},


Is there a particular reason to use METH_FASTCALL here? Can't we just use METH_VARARGS? Did you use it because it was easy to reproduce the crash in Python 3?

Also, if it's not possible to crash the interpreter in a pure Python 3 code, I wonder whether we should only fix this in Python 2.7.

if it's not possible to crash the interpreter in a pure Python 3 code

I never said that it's impossible. I just said that I didn't manage to. There is a lot of C code in CPython, I didn't audit all of it to ensure that it can't be used to get this crash. Even if I did, somebody might add a function tomorrow which does allow to induce the crash.

Is there a particular reason to use METH_FASTCALL here?

Absolutely. When calling with METH_VARARGS, a temporary tuple is constructed which holds an additional reference. So there won't be a crash in that case. I'm guessing that this is also the reason why the crash is more likely to occur on Python 2.7.

berkerpeksag · 2018-07-05T12:33:17Z

Lib/test/test_descr.py

+            descr = Descr()
+            def __new__(cls):
+                cls.descr = None
+                cls.lst = [2**i for i in range(10000)]


I'm at work at the moment, so I don't have a chance to test this, but do we need cls.lst to reproduce the crash?

do we need cls.lst to reproduce the crash?

That line is just to mess with the memory to ensure a sufficiently corrupted state.

jdemeyer · 2018-07-05T13:51:12Z

If you insist, here is a pure Python 3 reproducer:

import pickle

class Descr:
    __get__ = pickle.dump
    __set__ = None

class M(type):
    def __int__(self):
        self.write = None
        return 0

class X(type, metaclass=M):
    write = Descr()

class Y(metaclass=X):
    pass

Y.write

However, this code is so artificial that it would be really strange to add it to the testsuite. I think that the current C code does a much better job of showing the error.

jdemeyer · 2018-07-05T14:50:30Z

I made the requested changes on the branch.

jdemeyer · 2018-07-29T09:49:43Z

if it's not possible to crash the interpreter in a pure Python 3 code, I wonder whether we should only fix this in Python 2.7.

First of all, it is possible to crash the interpreter using pickle.dump as I demonstrated above.

Second, CPython is more than just the Python interpreter: even if a bug is only relevant for the Python/C API, it's still a bug that should be fixed.

So we can get this merged finally? It's an obvious bug fix, and I see nothing controversial about it.

jdemeyer · 2018-09-04T21:36:00Z

@vstinner You obviously care about abusing borrowed references...

vstinner

The typeobject.c fix LGTM, but I'm not sure about the test. The test seems quite complicated and not reliable :-( Do we really have to add an unit test for that?

@serhiy-storchaka: Would you be ok to omit the test on such fix?

I dislike having a test doing "cls.descr = None" and "Create this large list to corrupt some unused memory". IMHO the test is too close to the exact C implementation.

jdemeyer · 2018-09-05T09:07:32Z

I added a testcase by request of @berkerpeksag

jdemeyer · 2018-09-05T09:49:08Z

I dislike having a test doing "cls.descr = None"

Why? That is exactly the main point of the test (unless you prefer del cls.descr?). Only the "Create this large list to corrupt some unused memory" part is hackish indeed.

vstinner · 2018-09-05T15:26:26Z

I dislike having a test doing "cls.descr = None"

Why? That is exactly the main point of the test (unless you prefer del cls.descr?).

If it becomes part of the test suite, it means that we have to support this weird use case. I'm not sure that it should be part of the "language specification".

berkerpeksag · 2018-09-05T20:08:00Z

The whole point of this issue and PR is that someone is already depending on this behavior. We already have tests for more rare cases (e.g. https://bugs.python.org/issue31781) than this one.

We could wrap the test with the @cpython_only decorator if you think that the test is too close to the implementation.

jdemeyer · 2018-09-06T12:34:32Z

I'm not sure that it should be part of the "language specification".

So you think that doing cls.descr = None is not part of the language specification? I would guess that adding/changing attributes of classes like that is a reasonable standard feature.

vstinner · 2018-09-06T14:30:25Z

I have no strong opinion on this change. I let someone else take a decision.

When calling tp_descr_get(self, obj, type), make sure that we own a reference to "self"

jdemeyer · 2018-09-06T18:39:23Z

Since the testcase is apparently controversial, I decided to separate it as a new pull request: #9084

I really hope that this issue can now finally be fixed.

miss-islington · 2018-09-07T07:37:03Z

Thanks @jdemeyer for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖

When calling tp_descr_get(self, obj, type), make sure that we own a strong reference to "self". (cherry picked from commit 8f73548) Co-authored-by: jdemeyer <[email protected]>

bedevere-bot · 2018-09-07T07:37:19Z

GH-9087 is a backport of this pull request to the 3.7 branch.

bedevere-bot · 2018-09-07T07:37:28Z

GH-9088 is a backport of this pull request to the 3.6 branch.

When calling tp_descr_get(self, obj, type), make sure that we own a strong reference to "self". (cherry picked from commit 8f73548) Co-authored-by: jdemeyer <[email protected]>

bedevere-bot · 2018-09-07T07:37:36Z

GH-9089 is a backport of this pull request to the 2.7 branch.

When calling tp_descr_get(self, obj, type), make sure that we own a strong reference to "self". (cherry picked from commit 8f73548) Co-authored-by: jdemeyer <[email protected]>

vstinner · 2018-09-07T07:38:55Z

Since the testcase is apparently controversial, I decided to separate it as a new pull request: #9084

I'm sorry for being picky :-(

I really hope that this issue can now finally be fixed.

Sure, I just merged it. miss-inlington bot created the 3 backports and I already approved them, they will be merged once the CI tests pass.

When calling tp_descr_get(self, obj, type), make sure that we own a strong reference to "self". (cherry picked from commit 8f73548) Co-authored-by: jdemeyer <[email protected]>

When calling tp_descr_get(self, obj, type), make sure that we own a strong reference to "self". (cherry picked from commit 8f73548)

the-knights-who-say-ni added the CLA not signed label Mar 14, 2018

bedevere-bot added the awaiting review label Mar 14, 2018

jdemeyer force-pushed the bpo-25750 branch from 64efcbf to 980678c Compare March 14, 2018 20:43

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Mar 14, 2018

berkerpeksag added needs backport to 3.6 labels Mar 23, 2018

berkerpeksag reviewed Mar 23, 2018

View reviewed changes

jdemeyer force-pushed the bpo-25750 branch from 980678c to 4b18c58 Compare April 20, 2018 13:58

timokau mentioned this pull request Apr 26, 2018

python-2.7.14: Fix #27177 and #25750 NixOS/nixpkgs#39555

Closed

8 tasks

berkerpeksag reviewed Jul 5, 2018

View reviewed changes

jdemeyer force-pushed the bpo-25750 branch from 4b18c58 to 47a997b Compare July 5, 2018 13:08

waynr mentioned this pull request Jul 6, 2018

Vendor python type_getattro patch. NixOS/nixpkgs#43112

Closed

9 tasks

jdemeyer force-pushed the bpo-25750 branch from 47a997b to 6c02ef4 Compare September 4, 2018 21:40

vstinner reviewed Sep 5, 2018

View reviewed changes

bpo-25750: fix refcounts in type_getattro()

6ddb833

When calling tp_descr_get(self, obj, type), make sure that we own a reference to "self"

jdemeyer force-pushed the bpo-25750 branch from 6c02ef4 to 6ddb833 Compare September 6, 2018 18:34

jdemeyer mentioned this pull request Sep 6, 2018

bpo-25750: add testcase on bad descriptor __get__() #9084

Merged

vstinner merged commit 8f73548 into python:master Sep 7, 2018

bedevere-bot removed the awaiting review label Sep 7, 2018

bedevere-bot removed needs backport to 3.7 labels Sep 7, 2018

bedevere-bot removed the needs backport to 2.7 label Sep 7, 2018

vstinner added a commit that referenced this pull request Sep 7, 2018

bpo-25750: fix refcounts in type_getattro() (GH-6118) (GH-9091)

bf2bd8f

When calling tp_descr_get(self, obj, type), make sure that we own a strong reference to "self". (cherry picked from commit 8f73548)

jdemeyer deleted the bpo-25750 branch August 8, 2019 11:22

		@@ -0,0 +1,2 @@
		Fix rare Python crash due to bad refcounting in ``type_getattro()`` if a
		descriptor deletes itself from the class.

Uh oh!

bpo-25750: fix refcounts in type_getattro() #6118

bpo-25750: fix refcounts in type_getattro() #6118

Uh oh!

Conversation

jdemeyer commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Mar 14, 2018

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Mar 24, 2018

Uh oh!

berkerpeksag commented Mar 24, 2018

Uh oh!

jdemeyer commented Apr 2, 2018

Uh oh!

jdemeyer commented Apr 20, 2018

Uh oh!

jdemeyer commented Apr 20, 2018

Uh oh!

jdemeyer commented Jul 5, 2018

Uh oh!

berkerpeksag Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

jdemeyer Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

jdemeyer Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

jdemeyer Jul 5, 2018

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Jul 5, 2018

Uh oh!

jdemeyer commented Jul 5, 2018

Uh oh!

jdemeyer commented Jul 29, 2018

Uh oh!

jdemeyer commented Sep 4, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Sep 5, 2018

Uh oh!

jdemeyer commented Sep 5, 2018

Uh oh!

vstinner commented Sep 5, 2018

Uh oh!

berkerpeksag commented Sep 5, 2018

Uh oh!

jdemeyer commented Sep 6, 2018

Uh oh!

vstinner commented Sep 6, 2018

Uh oh!

jdemeyer commented Sep 6, 2018

Uh oh!

miss-islington commented Sep 7, 2018

Uh oh!

bedevere-bot commented Sep 7, 2018

Uh oh!

bedevere-bot commented Sep 7, 2018

jdemeyer commented Mar 14, 2018 •

edited

Loading

jdemeyer Jul 5, 2018 •

edited

Loading