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!

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.

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.

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.

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.

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

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.

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

I added a testcase by request of @berkerpeksag

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.

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

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

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

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

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.