bpo-39382: Avoid dangling object use in abstract_issubclass() #18530

Jongy · 2020-02-16T21:19:33Z

Take a reference of __bases__ tuple item before releasing the tuple reference,
as it may destroy the tuple, possibly destroying (some of) its items.

@serhiy-storchaka, I will look into your suggestion in the bpo and see if I can reproduce it with a short snippet, so I can add tests for it. I will also think of some way to simplify the code, this function became too complicated with this change, perhaps it's time to break it?

Therefore, opening as a draft for the meantime.

https://bugs.python.org/issue39382

Take a reference of __bases__ tuple item before releasing the tuple reference, as it may destroy the tuple, possibly destroying (some of) its items.

pppery · 2020-02-17T16:47:41Z

Does

class FakeClass:
    def __init__(self,base=False):
        self.base = base
    @property
    def __bases__(self):
        if self.base:
            return ()
        return (FakeClass(True),)
issubclass(FakeClass(), int)

work as a test case? It crashes for me on Python 3.7

Jongy · 2020-02-17T19:17:25Z

Yeah, it appears to exhibit the same outcome. It crashes on master (4c1b6a6) but doesn't crash on this branch. Thanks for that, @pppery !

Well, this snippet is much less complicated than what I was trying to achieve (as I described in the bpo, I encountered this while using rpyc so I tried to imitate rpyc's netref objects with the metaclasses magics).

I don't understand though, while it exhibits the bug, this doesn't:

class A:
    pass

class B:
    @property
    def __bases__(self):
        return (A(), )

print(issubclass(B(), int))

I've added prints in abstract_issubclass before it modifies the refcounts. In both cases (your example, and the one above) the retrieved bases tuple has 1 reference, and the object inside it has 1 reference as well. In both cases, after the Py_DECREF, the derived object has refcount of 0. But yours crashes and mine does not?

pppery · 2020-02-17T19:33:32Z

I don't know why that works either.

serhiy-storchaka · 2020-02-17T20:47:40Z

I get a crash with both examples.

Jongy · 2020-02-17T23:03:35Z

Weird. My assumption is that in the second example, another object gets allocated in the exact, same memory chunk, and so abstract_issubclass continues operating on it and not on poisoned memory. But I haven't verified that.

Anyway, I'll add a test using the first example, and a NEWS entry.

Jongy · 2020-02-17T23:31:21Z

The following snippet crashes:

class A:
    @property
    def __bases__(self):
        return (int, )


class B:
    @property
    def __bases__(self):
        return (A(), )


assert issubclass(B(), int)

(and the assert passes on this branch)

I'll add a test based on it.

Jongy · 2020-02-17T23:48:26Z

The following snippet crashes

In the test_isinstance.py file it didn't. When I added a dummy member to the class, it did. I guess it really has something to do with the class object size and its memory being reused.

Jongy · 2020-02-18T00:06:03Z

Objects/abstract.c

@@ -2377,11 +2377,17 @@ abstract_issubclass(PyObject *derived, PyObject *cls)
    PyObject *bases = NULL;
    Py_ssize_t i, n;
    int r = 0;
+    int derived_ref = 0;


Perhaps just take a ref of derived before the loop, and remove this variable and its checks altogether? My intention with derived_ref was to avoid unnecessary memory writes to a shared/common location.

Yes, it may simplify the code, at cost of making it slightly slower. But this is not performance critical code, it is only called when any of arguments is not a type (very unusual case).

Alternatively, you can keep a reference to bases until you get a new bases. Py_SETREF may help to keep the code shorter: Py_SETREF(bases, abstract_get_bases(derived)), but it may still look less obvious.

Left it on to you. All variants are good enough.

Py_SETREF seems nice. I didn't know of this macro. I'll use it.

It is not public. Don't use it in third-party code. It is safer to copy its definition under other name if you need it.

Sorry, I don't think I understood that last comment of yours. What do you mean by third-party code? Using it here is okay, right?

In your own code, in the code of your company or in the code of other projects except CPython.

It is not the part of the public C API and can be renamed, changed or removed in new Python versions.

Mhm, I see. Thanks for the tip!

serhiy-storchaka

LGTM. Just added suggestion to reword the NEWS entry.

Lib/test/test_isinstance.py

serhiy-storchaka · 2020-02-18T06:14:16Z

Objects/abstract.c

@@ -2377,11 +2377,17 @@ abstract_issubclass(PyObject *derived, PyObject *cls)
    PyObject *bases = NULL;
    Py_ssize_t i, n;
    int r = 0;
+    int derived_ref = 0;


Yes, it may simplify the code, at cost of making it slightly slower. But this is not performance critical code, it is only called when any of arguments is not a type (very unusual case).

Alternatively, you can keep a reference to bases until you get a new bases. Py_SETREF may help to keep the code shorter: Py_SETREF(bases, abstract_get_bases(derived)), but it may still look less obvious.

Left it on to you. All variants are good enough.

serhiy-storchaka · 2020-02-18T06:32:34Z

Misc/NEWS.d/next/Core and Builtins/2020-02-18-01-40-13.bpo-39382.OLSJu9.rst

@@ -0,0 +1,3 @@
+Hold reference to ``__bases__`` tuple item before releasing the tuple itself
+in ``abstract_issubclass()``, avoiding a use-after-free in the next loop


abstract_issubclass() is an implementation detail. Python users do not know anything about it. Use things which expose the bug on the Python level: issubclass().

Also "hold reference to __bases__ tuple item" says how the bug has been fixed. Instead describe what the bug looked itself from the point of view of the common Python user (a crash in issubclass() when __bases__ contains the only reference to its item, or something like).

Gotcha, will change it to a more user-facing message.

serhiy-storchaka

Great!

Apologies, but I forgot about one tiny detail. Do you mind to add your name in Misc/ACKS and add the sentence "Patch by yourname." to the end of the NEWS entry?

Lib/test/test_isinstance.py

serhiy-storchaka · 2020-02-18T19:49:06Z

Objects/abstract.c

@@ -2377,11 +2377,17 @@ abstract_issubclass(PyObject *derived, PyObject *cls)
    PyObject *bases = NULL;
    Py_ssize_t i, n;
    int r = 0;
+    int derived_ref = 0;


It is not public. Don't use it in third-party code. It is safer to copy its definition under other name if you need it.

codecov · 2020-02-18T20:00:12Z

Codecov Report

Merging #18530 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #18530      +/-   ##
==========================================
- Coverage   82.20%   82.11%   -0.09%     
==========================================
  Files        1958     1955       -3     
  Lines      589743   584066    -5677     
  Branches    44457    44457              
==========================================
- Hits       484772   479631    -5141     
+ Misses      95308    94788     -520     
+ Partials     9663     9647      -16

Impacted Files	Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py	`30.00% <0.00%> (-65.00%)`	⬇️
Lib/distutils/command/bdist_rpm.py	`7.63% <0.00%> (-56.88%)`	⬇️
Lib/test/test_urllib2net.py	`76.92% <0.00%> (-13.85%)`	⬇️
Lib/test/test_smtpnet.py	`78.57% <0.00%> (-12.86%)`	⬇️
Lib/ftplib.py	`63.85% <0.00%> (-6.06%)`	⬇️
Lib/test/test_ftplib.py	`87.11% <0.00%> (-4.72%)`	⬇️
Tools/scripts/db2pickle.py	`17.82% <0.00%> (-3.97%)`	⬇️
Tools/scripts/pickle2db.py	`16.98% <0.00%> (-3.78%)`	⬇️
Lib/test/test_socket.py	`71.94% <0.00%> (-3.87%)`	⬇️
Lib/test/test_asyncio/test_base_events.py	`91.84% <0.00%> (-3.45%)`	⬇️
... and 326 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c33bdbb...f71d08d. Read the comment docs.

Jongy · 2020-02-18T20:00:21Z

Apologies, but I forgot about one tiny detail. Do you mind to add your name in Misc/ACKS and add the sentence "Patch by yourname." to the end of the NEWS entry?

Ofc, will add.

Not sure about this Mac failure. I don't think it's related to my change.

About the commit message for the squash: The first commit message is not entirely relevant anymore, so I'm thinking something about:

[bpo-39382](https://bugs.python.org/issue39382): Avoid dangling object use in abstract_issubclass()

Hold reference of __bases__ tuple until tuple item is done with, because by
dropping the reference the item may be destroyed.

serhiy-storchaka · 2020-02-18T20:07:28Z

Ignore the failure on macOS. It is not related.

Jongy · 2020-02-19T08:52:33Z

@serhiy-storchaka anything else needed, or it looks ready to you?

serhiy-storchaka · 2020-02-22T13:15:28Z

Thank you for your contribution @Jongy. It was a pleasure to review your PR, and I hope this is not a last your PR to CPython.

Thank you for your example @pppery. It helped a lot!

miss-islington · 2020-02-22T13:16:02Z

Thanks @Jongy for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2020-02-22T13:16:02Z

Thanks @Jongy for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

…GH-18530) Hold reference of __bases__ tuple until tuple item is done with, because by dropping the reference the item may be destroyed. (cherry picked from commit 1c56f8f) Co-authored-by: Yonatan Goldschmidt <yon.goldschmidt@gmail.com>

bedevere-bot · 2020-02-22T13:16:27Z

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

bedevere-bot · 2020-02-22T13:16:34Z

GH-18607 is a backport of this pull request to the 3.8 branch.

Hold reference of __bases__ tuple until tuple item is done with, because by dropping the reference the item may be destroyed. (cherry picked from commit 1c56f8f) Co-authored-by: Yonatan Goldschmidt <yon.goldschmidt@gmail.com>

Jongy · 2020-02-22T13:58:44Z

Thanks @serhiy-storchaka :)

bpo-39382: Avoid dangling object use in abstract_issubclass()

689b728

Take a reference of __bases__ tuple item before releasing the tuple reference, as it may destroy the tuple, possibly destroying (some of) its items.

the-knights-who-say-ni added the CLA signed label Feb 16, 2020

bedevere-bot added the awaiting review label Feb 16, 2020

Jongy requested a review from serhiy-storchaka February 16, 2020 21:19

bpo-39382: Add a test for the crash and a NEWS.d entry

a3fcf28

Jongy marked this pull request as ready for review February 17, 2020 23:48

Jongy commented Feb 18, 2020

View reviewed changes

serhiy-storchaka reviewed Feb 18, 2020

View reviewed changes

Jongy added 3 commits February 18, 2020 20:34

bpo-39382: Rewrite NEWS entry using higher-level details

79b8c52

bpo-39382: Use Py_XSETREF to keep the code simpler

dd8328f

bpo-39382: Add test comment about the set member variable

9d7f197

Jongy force-pushed the bpo-39382-abstract_issubclass-refcount-fix branch from d74f8bb to 9d7f197 Compare February 18, 2020 19:17

serhiy-storchaka reviewed Feb 18, 2020

View reviewed changes

bpo-39382: Update ACKS and NEWS entry with my name

f71d08d

serhiy-storchaka approved these changes Feb 22, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 22, 2020

serhiy-storchaka merged commit 1c56f8f into python:master Feb 22, 2020

bedevere-bot removed the awaiting merge label Feb 22, 2020

serhiy-storchaka added needs backport to 3.7 needs backport to 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels Feb 22, 2020

bedevere-bot removed the needs backport to 3.7 label Feb 22, 2020

bedevere-bot removed the needs backport to 3.8 only security fixes label Feb 22, 2020

Jongy deleted the bpo-39382-abstract_issubclass-refcount-fix branch February 22, 2020 14:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-39382: Avoid dangling object use in abstract_issubclass() #18530

bpo-39382: Avoid dangling object use in abstract_issubclass() #18530

Jongy commented Feb 16, 2020 •

edited

Loading

pppery commented Feb 17, 2020 •

edited

Loading

Jongy commented Feb 17, 2020

pppery commented Feb 17, 2020

serhiy-storchaka commented Feb 17, 2020

Jongy commented Feb 17, 2020

Jongy commented Feb 17, 2020

Jongy commented Feb 17, 2020

Jongy Feb 18, 2020

serhiy-storchaka Feb 18, 2020

Jongy Feb 18, 2020

serhiy-storchaka Feb 18, 2020

Jongy Feb 18, 2020

serhiy-storchaka Feb 18, 2020

Jongy Feb 18, 2020

serhiy-storchaka left a comment

serhiy-storchaka Feb 18, 2020

serhiy-storchaka Feb 18, 2020

Jongy Feb 18, 2020

serhiy-storchaka left a comment

serhiy-storchaka Feb 18, 2020

codecov bot commented Feb 18, 2020 •

edited

Loading

Jongy commented Feb 18, 2020 •

edited by bedevere-bot

Loading

serhiy-storchaka commented Feb 18, 2020

Jongy commented Feb 19, 2020 •

edited

Loading

serhiy-storchaka commented Feb 22, 2020

miss-islington commented Feb 22, 2020

miss-islington commented Feb 22, 2020

bedevere-bot commented Feb 22, 2020

bedevere-bot commented Feb 22, 2020

Jongy commented Feb 22, 2020

		@@ -0,0 +1,3 @@
		Hold reference to ``__bases__`` tuple item before releasing the tuple itself
		in ``abstract_issubclass()``, avoiding a use-after-free in the next loop

bpo-39382: Avoid dangling object use in abstract_issubclass() #18530

bpo-39382: Avoid dangling object use in abstract_issubclass() #18530

Conversation

Jongy commented Feb 16, 2020 • edited Loading

pppery commented Feb 17, 2020 • edited Loading

Jongy commented Feb 17, 2020

pppery commented Feb 17, 2020

serhiy-storchaka commented Feb 17, 2020

Jongy commented Feb 17, 2020

Jongy commented Feb 17, 2020

Jongy commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2020 • edited Loading

Codecov Report

Jongy commented Feb 18, 2020 • edited by bedevere-bot Loading

serhiy-storchaka commented Feb 18, 2020

Jongy commented Feb 19, 2020 • edited Loading

serhiy-storchaka commented Feb 22, 2020

miss-islington commented Feb 22, 2020

miss-islington commented Feb 22, 2020

bedevere-bot commented Feb 22, 2020

bedevere-bot commented Feb 22, 2020

Jongy commented Feb 22, 2020

Jongy commented Feb 16, 2020 •

edited

Loading

pppery commented Feb 17, 2020 •

edited

Loading

codecov bot commented Feb 18, 2020 •

edited

Loading

Jongy commented Feb 18, 2020 •

edited by bedevere-bot

Loading

Jongy commented Feb 19, 2020 •

edited

Loading