bpo-25750: fix refcounts in type_getattro() by jdemeyer · Pull Request #6118 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation34 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

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.

try:
from _testcapi import bad_get
except ImportError:
return

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "Patch by Jeroen Demeyer."

@@ -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},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need cls.lst to reproduce the crash?

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

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.

I made the requested changes on the branch.

waynr mentioned this pull request

Jul 6, 2018

9 tasks

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

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

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.

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.

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"

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.

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.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 7, 2018

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 jdemeyer@cage.ugent.be

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 7, 2018

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 jdemeyer@cage.ugent.be

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 7, 2018

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 jdemeyer@cage.ugent.be

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.

miss-islington added a commit that referenced this pull request

Sep 7, 2018

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 jdemeyer@cage.ugent.be

vstinner pushed a commit that referenced this pull request

Sep 7, 2018

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 jdemeyer@cage.ugent.be

vstinner added a commit that referenced this pull request

Sep 7, 2018

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

(cherry picked from commit 8f73548)