classification
Title: abstract_issubclass() doesn't take bases tuple item ref
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Yonatan Goldschmidt, miss-islington, nascheme, ppperry, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-01-18 17:30 by Yonatan Goldschmidt, last changed 2020-02-22 14:23 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
abstract_issubclass_refcount_fix.diff Yonatan Goldschmidt, 2020-01-18 17:30 abstract_issubclass() bases tuple item refcount fix
Pull Requests
URL Status Linked Edit
PR 18530 merged Yonatan Goldschmidt, 2020-02-16 21:19
PR 18606 merged miss-islington, 2020-02-22 13:16
PR 18607 merged miss-islington, 2020-02-22 13:16
Messages (6)
msg360247 - (view) Author: Yonatan Goldschmidt (Yonatan Goldschmidt) * Date: 2020-01-18 17:30
I encountered a crash using rpyc. Since rpyc is pure-Python, I guessed the problem is with Python itself.

Originally tested on v3.7, the rest of this issue is based on v3.9.0a2 (compiling on an Ubuntu 18.04 docker).

I narrowed down the rpyc-based snippet to this:

    # server side
    class X: pass
    x_instance = X()

    from rpyc.core import SlaveService
    from rpyc.utils.classic import DEFAULT_SERVER_PORT
    from rpyc.utils.server import ThreadedServer
    t = ThreadedServer(SlaveService, port=DEFAULT_SERVER_PORT, reuse_addr=True)
    t.start()

    # client side
    import rpyc
    conn = rpyc.classic.connect("localhost")
    x = conn.modules.__main__.x_instance
    y = x.__class__
    issubclass(y, int)

Client side gets a SIGSEGV in `_PyObject_LookupAttr`, dereferencing an invalid `tp` pointer read from a posioned `v` object.

After some reference count debugging, I found that for the rpyc `y` object (in the client code), accessing `__bases__` returns a tuple with refcount=1, and it has a single item whose refcount is 1 as well.

abstract_issubclass() calls abstract_get_bases() to get this refcount=1 tuple, and in the fastpath for single inheritance (tuple size = 1) it loads the single item from the tuple (`derived = PyTuple_GET_ITEM(bases, 0)`) and then decrements the refcount on the tuple, effectively deallocating the tuple and the `derived` object (whose only reference was from the tuple).

I tried to mimic the Python magic rpyc does to get the same crash without rpyc, and reached the following snippet (which doesn't exhibit the problem):

    class Meta(type):
        def __getattribute__(self, attr):
            if attr == "__bases__":
                class Base: pass
                return (Base, )
            return type.__getattribute__(self, attr)

    class X(metaclass=Meta):
        pass

    issubclass(X().__class__, int)

In this case, the refcount is decremented from 4 to 3 as abstract_issubclass() gets rid of the tuple (instead of from 1 to 0 as happens in the rpyc case). I don't know how rpyc does it.

Attached is a patch that solves the problem (takes a ref of the tuple item before releasing the ref of the tuple itself).
I'm not sure this change is worth the cost because, well, I don't fully understand the severity of it since I couldn't reproduce it without using rpyc. I assume dynamically-created, unreferenced `__bases__` tuples as I have here are not so common.

Anyway, if you do decide it's worth it, I'd be happy to improve the patch (it's quite messy the way this function is structured) and post it to GitHub :)

Yonatan
msg362050 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-02-16 07:53
Thank you for your report and patch. Agree that the code does not look safe. Do you mind to create a pull request?

Would be nice to add a test for it. The three references to the Python class are:

* the __dict__ descriptor
* the __weakref__ descriptor
* the __mro__ tuple

You can get rid of the first two by setting __slots__ = () in the class definition. But it is not so easy with the __mro__ tuple. I will try more.
msg362155 - (view) Author: (ppperry) Date: 2020-02-17 18:38
I posted a test on the PR
msg362463 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-02-22 13:11
New changeset 1c56f8ffad44478b4214a2bf8eb7cf51c28a347a by Yonatan Goldschmidt in branch 'master':
bpo-39382: Avoid dangling object use in abstract_issubclass() (GH-18530)
https://github.com/python/cpython/commit/1c56f8ffad44478b4214a2bf8eb7cf51c28a347a
msg362464 - (view) Author: miss-islington (miss-islington) Date: 2020-02-22 13:32
New changeset 43a0137c87b997c6ba8b23cc3281ce2de18f008a by Miss Islington (bot) in branch '3.7':
bpo-39382: Avoid dangling object use in abstract_issubclass() (GH-18530)
https://github.com/python/cpython/commit/43a0137c87b997c6ba8b23cc3281ce2de18f008a
msg362465 - (view) Author: miss-islington (miss-islington) Date: 2020-02-22 13:34
New changeset 0c1827e70c1c05ce1982a34380cea7d391904293 by Miss Islington (bot) in branch '3.8':
bpo-39382: Avoid dangling object use in abstract_issubclass() (GH-18530)
https://github.com/python/cpython/commit/0c1827e70c1c05ce1982a34380cea7d391904293
History
Date User Action Args
2020-02-22 14:23:09serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2020-02-22 13:34:09miss-islingtonsetmessages: + msg362465
2020-02-22 13:32:39miss-islingtonsetnosy: + miss-islington
messages: + msg362464
2020-02-22 13:16:32miss-islingtonsetpull_requests: + pull_request17973
2020-02-22 13:16:25miss-islingtonsetpull_requests: + pull_request17972
2020-02-22 13:11:55serhiy.storchakasetmessages: + msg362463
2020-02-17 18:38:19ppperrysetnosy: + ppperry
messages: + msg362155
2020-02-16 21:19:34Yonatan Goldschmidtsetstage: patch review
pull_requests: + pull_request17906
2020-02-16 07:53:55serhiy.storchakasetmessages: + msg362050
versions: + Python 3.7, Python 3.8
2020-02-16 06:23:46serhiy.storchakasetnosy: + serhiy.storchaka
2020-02-16 04:11:57shihai1991setnosy: + nascheme
2020-01-18 17:30:12Yonatan Goldschmidtcreate