From 18657330c9dbe7b4738378d85e2b06993be262f9 Mon Sep 17 00:00:00 2001 From: "Jeong, YunWon" Date: Tue, 17 Mar 2026 21:00:02 +0900 Subject: [PATCH] Drop old PyObjectRef outside type lock to prevent deadlock Dropping values inside with_type_lock can trigger weakref callbacks, which may access attributes (LOAD_ATTR specialization) and re-acquire the non-reentrant type mutex, causing deadlock. Return old values from lock closures so they drop after lock release. --- .github/workflows/cron-ci.yaml | 18 ++--- Lib/test/test_class.py | 2 + crates/vm/src/builtins/type.rs | 95 ++++++++++++----------- crates/vm/src/stdlib/_ctypes/base.rs | 10 +-- crates/vm/src/stdlib/_ctypes/structure.rs | 6 +- crates/vm/src/stdlib/_ctypes/union.rs | 3 +- extra_tests/custom_text_test_runner.py | 54 ++++++------- 7 files changed, 95 insertions(+), 93 deletions(-) diff --git a/.github/workflows/cron-ci.yaml b/.github/workflows/cron-ci.yaml index 68ef4dea4..a481e71de 100644 --- a/.github/workflows/cron-ci.yaml +++ b/.github/workflows/cron-ci.yaml @@ -12,7 +12,7 @@ on: name: Periodic checks/tasks env: - CARGO_ARGS: --no-default-features --features stdlib,importlib,encodings,ssl-rustls,jit + CARGO_ARGS: --no-default-features --features stdlib,importlib,stdio,encodings,ssl-rustls,jit,host_env PYTHON_VERSION: "3.14.3" jobs: @@ -35,7 +35,7 @@ jobs: python-version: ${{ env.PYTHON_VERSION }} - run: sudo apt-get update && sudo apt-get -y install lcov - name: Run cargo-llvm-cov with Rust tests. - run: cargo llvm-cov --no-report --workspace --exclude rustpython_wasm --exclude rustpython-compiler-source --exclude rustpython-venvlauncher --verbose --no-default-features --features stdlib,importlib,encodings,ssl-rustls,jit + run: cargo llvm-cov --no-report --workspace --exclude rustpython_wasm --exclude rustpython-compiler-source --exclude rustpython-venvlauncher --verbose --no-default-features --features stdlib,importlib,stdio,encodings,ssl-rustls,jit,host_env - name: Run cargo-llvm-cov with Python snippets. run: python scripts/cargo-llvm-cov.py continue-on-error: true @@ -48,7 +48,7 @@ jobs: if: ${{ github.event_name != 'pull_request' }} uses: codecov/codecov-action@v5 with: - file: ./codecov.lcov + files: ./codecov.lcov testdata: name: Collect regression test data @@ -170,12 +170,12 @@ jobs: - name: restructure generated files run: | cd ./target/criterion/reports - find -type d -name cpython | xargs rm -rf - find -type d -name rustpython | xargs rm -rf - find -mindepth 2 -maxdepth 2 -name violin.svg | xargs rm -rf - find -type f -not -name violin.svg | xargs rm -rf - for file in $(find -type f -name violin.svg); do mv $file $(echo $file | sed -E "s_\./([^/]+)/([^/]+)/violin\.svg_./\1/\2.svg_"); done - find -mindepth 2 -maxdepth 2 -type d | xargs rm -rf + find . -type d -name cpython -print0 | xargs -0 rm -rf + find . -type d -name rustpython -print0 | xargs -0 rm -rf + find . -mindepth 2 -maxdepth 2 -name violin.svg -print0 | xargs -0 rm -rf + find . -type f -not -name violin.svg -print0 | xargs -0 rm -rf + find . -type f -name violin.svg -exec sh -c 'for file; do mv "$file" "$(echo "$file" | sed -E "s_\./([^/]+)/([^/]+)/violin\.svg_./\1/\2.svg_")"; done' _ {} + + find . -mindepth 2 -maxdepth 2 -type d -print0 | xargs -0 rm -rf cd .. mv reports/* . rmdir reports diff --git a/Lib/test/test_class.py b/Lib/test/test_class.py index 7420f289b..ea8653cd0 100644 --- a/Lib/test/test_class.py +++ b/Lib/test/test_class.py @@ -1,5 +1,6 @@ "Test the functionality of Python classes implementing operators." +import sys import unittest from test import support from test.support import cpython_only, import_helper, script_helper @@ -614,6 +615,7 @@ class ClassTests(unittest.TestCase): with self.assertRaises(TypeError): a >= b + @unittest.skipIf(sys.platform == "win32", "TODO: RUSTPYTHON; flaky on Windows") def testHashComparisonOfMethods(self): # Test comparison and hash of methods class A: diff --git a/crates/vm/src/builtins/type.rs b/crates/vm/src/builtins/type.rs index 3148ff420..dd937f98f 100644 --- a/crates/vm/src/builtins/type.rs +++ b/crates/vm/src/builtins/type.rs @@ -1562,18 +1562,19 @@ impl PyType { drop(attrs); let none = vm.ctx.none(); - Ok(Self::with_type_lock(vm, || { + let (result, _prev) = Self::with_type_lock(vm, || { let mut attrs = self.attributes.write(); if let Some(annotate) = attrs.get(annotate_key).cloned() { - return annotate; + return (annotate, None); } if let Some(annotate) = attrs.get(annotate_func_key).cloned() { - return annotate; + return (annotate, None); } self.modified_inner(); - attrs.insert(annotate_func_key, none.clone()); - none - })) + let prev = attrs.insert(annotate_func_key, none.clone()); + (none, prev) + }); + Ok(result) } #[pygetset(setter)] @@ -1596,13 +1597,16 @@ impl PyType { return Err(vm.new_type_error("__annotate__ must be callable or None")); } - Self::with_type_lock(vm, || { + let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); let mut attrs = self.attributes.write(); - if !vm.is_none(&value) { - attrs.swap_remove(identifier!(vm, __annotations_cache__)); - } - attrs.insert(identifier!(vm, __annotate_func__), value); + let removed = if !vm.is_none(&value) { + attrs.swap_remove(identifier!(vm, __annotations_cache__)) + } else { + None + }; + let prev = attrs.insert(identifier!(vm, __annotate_func__), value); + (removed, prev) }); Ok(()) @@ -1665,20 +1669,21 @@ impl PyType { vm.ctx.new_dict().into() }; - Ok(Self::with_type_lock(vm, || { + let (result, _prev) = Self::with_type_lock(vm, || { let mut attrs = self.attributes.write(); if let Some(existing) = attrs.get(annotations_key).cloned() && !existing.class().is(vm.ctx.types.getset_type) { - return existing; + return (existing, None); } if let Some(existing) = attrs.get(annotations_cache_key).cloned() { - return existing; + return (existing, None); } self.modified_inner(); - attrs.insert(annotations_cache_key, annotations.clone()); - annotations - })) + let prev = attrs.insert(annotations_cache_key, annotations.clone()); + (annotations, prev) + }); + Ok(result) } #[pygetset(setter)] @@ -1694,11 +1699,12 @@ impl PyType { ))); } - Self::with_type_lock(vm, || { + let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); let mut attrs = self.attributes.write(); let has_annotations = attrs.contains_key(identifier!(vm, __annotations__)); + let mut prev = Vec::new(); match value { crate::function::PySetterValue::Assign(value) => { let key = if has_annotations { @@ -1706,32 +1712,29 @@ impl PyType { } else { identifier!(vm, __annotations_cache__) }; - attrs.insert(key, value); + prev.extend(attrs.insert(key, value)); if has_annotations { - attrs.swap_remove(identifier!(vm, __annotations_cache__)); + prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__))); } } crate::function::PySetterValue::Delete => { let removed = if has_annotations { - attrs - .swap_remove(identifier!(vm, __annotations__)) - .is_some() + attrs.swap_remove(identifier!(vm, __annotations__)) } else { - attrs - .swap_remove(identifier!(vm, __annotations_cache__)) - .is_some() + attrs.swap_remove(identifier!(vm, __annotations_cache__)) }; - if !removed { + if removed.is_none() { return Err(vm.new_attribute_error("__annotations__")); } + prev.extend(removed); if has_annotations { - attrs.swap_remove(identifier!(vm, __annotations_cache__)); + prev.extend(attrs.swap_remove(identifier!(vm, __annotations_cache__))); } } } - attrs.swap_remove(identifier!(vm, __annotate_func__)); - attrs.swap_remove(identifier!(vm, __annotate__)); - Ok(()) + prev.extend(attrs.swap_remove(identifier!(vm, __annotate_func__))); + prev.extend(attrs.swap_remove(identifier!(vm, __annotate__))); + Ok(prev) })?; Ok(()) @@ -1765,11 +1768,12 @@ impl PyType { #[pygetset(setter)] fn set___module__(&self, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { self.check_set_special_type_attr(identifier!(vm, __module__), vm)?; - Self::with_type_lock(vm, || { + let _prev_values = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes - .write() - .insert(identifier!(vm, __module__), value); + let mut attributes = self.attributes.write(); + let removed = attributes.swap_remove(identifier!(vm, __firstlineno__)); + let prev = attributes.insert(identifier!(vm, __module__), value); + (removed, prev) }); Ok(()) } @@ -1896,9 +1900,9 @@ impl PyType { match value { PySetterValue::Assign(val) => { self.check_set_special_type_attr(key, vm)?; - Self::with_type_lock(vm, || { + let _prev_value = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes.write().insert(key, val.into()); + self.attributes.write().insert(key, val.into()) }); } PySetterValue::Delete => { @@ -1908,9 +1912,9 @@ impl PyType { self.slot_name() ))); } - Self::with_type_lock(vm, || { + let _prev_value = Self::with_type_lock(vm, || { self.modified_inner(); - self.attributes.write().shift_remove(&key); + self.attributes.write().shift_remove(&key) }); } } @@ -2519,11 +2523,11 @@ impl Py { // Check if we can set this special type attribute self.check_set_special_type_attr(identifier!(vm, __doc__), vm)?; - PyType::with_type_lock(vm, || { + let _prev_value = PyType::with_type_lock(vm, || { self.modified_inner(); self.attributes .write() - .insert(identifier!(vm, __doc__), value); + .insert(identifier!(vm, __doc__), value) }); Ok(()) @@ -2586,14 +2590,17 @@ impl SetAttr for PyType { } let assign = value.is_assign(); - Self::with_type_lock(vm, || { + // Drop old value OUTSIDE the type lock to avoid deadlock: + // dropping may trigger weakref callbacks → method calls → + // LOAD_ATTR specialization → version_for_specialization → type lock. + let _prev_value = Self::with_type_lock(vm, || { // Invalidate inline caches before modifying attributes. // This ensures other threads see the version invalidation before // any attribute changes, preventing use-after-free of cached descriptors. zelf.modified_inner(); if let PySetterValue::Assign(value) = value { - zelf.attributes.write().insert(attr_name, value); + Ok(zelf.attributes.write().insert(attr_name, value)) } else { let prev_value = zelf.attributes.write().shift_remove(attr_name); // TODO: swap_remove applicable? if prev_value.is_none() { @@ -2603,8 +2610,8 @@ impl SetAttr for PyType { attr_name, ))); } + Ok(prev_value) } - Ok(()) })?; if attr_name.as_wtf8().starts_with("__") && attr_name.as_wtf8().ends_with("__") { diff --git a/crates/vm/src/stdlib/_ctypes/base.rs b/crates/vm/src/stdlib/_ctypes/base.rs index c5714e7cc..0bfbe57bb 100644 --- a/crates/vm/src/stdlib/_ctypes/base.rs +++ b/crates/vm/src/stdlib/_ctypes/base.rs @@ -2554,11 +2554,10 @@ fn make_fields( } let new_descr = super::PyCField::new_from_field(fdescr, index, offset); - cls.as_object().set_attr( + cls.set_attr( vm.ctx.intern_str(fname.as_wtf8()), new_descr.to_pyobject(vm), - vm, - )?; + ); } Ok(()) @@ -2597,11 +2596,10 @@ pub(super) fn make_anon_fields(cls: &Py, vm: &VirtualMachine) -> PyResul let mut new_descr = super::PyCField::new_from_field(descr, 0, 0); new_descr.set_anonymous(true); - cls.as_object().set_attr( + cls.set_attr( vm.ctx.intern_str(fname.as_wtf8()), new_descr.to_pyobject(vm), - vm, - )?; + ); make_fields(cls, descr, descr.index, descr.offset, vm)?; } diff --git a/crates/vm/src/stdlib/_ctypes/structure.rs b/crates/vm/src/stdlib/_ctypes/structure.rs index 0b310db07..c9ab92056 100644 --- a/crates/vm/src/stdlib/_ctypes/structure.rs +++ b/crates/vm/src/stdlib/_ctypes/structure.rs @@ -498,11 +498,7 @@ impl PyCStructType { }; // Set the CField as a class attribute - cls.as_object().set_attr( - vm.ctx.intern_str(name.clone()), - c_field.to_pyobject(vm), - vm, - )?; + cls.set_attr(vm.ctx.intern_str(name.clone()), c_field.to_pyobject(vm)); // Update tracking - don't advance offset for packed bitfields if field_advances_offset { diff --git a/crates/vm/src/stdlib/_ctypes/union.rs b/crates/vm/src/stdlib/_ctypes/union.rs index 10ef3ba57..c1882141e 100644 --- a/crates/vm/src/stdlib/_ctypes/union.rs +++ b/crates/vm/src/stdlib/_ctypes/union.rs @@ -312,8 +312,7 @@ impl PyCUnionType { PyCField::new(name.clone(), field_type_ref, 0, size as isize, index) }; - cls.as_object() - .set_attr(vm.ctx.intern_str(name), c_field.to_pyobject(vm), vm)?; + cls.set_attr(vm.ctx.intern_str(name), c_field.to_pyobject(vm)); } // Calculate total_align and aligned_size diff --git a/extra_tests/custom_text_test_runner.py b/extra_tests/custom_text_test_runner.py index afec493a6..3457bdfd0 100644 --- a/extra_tests/custom_text_test_runner.py +++ b/extra_tests/custom_text_test_runner.py @@ -38,6 +38,16 @@ from functools import reduce from unittest.runner import registerResult, result +def _get_method_dict(test): + """Get the __dict__ of the underlying function for a test method. + + Works for both bound methods (__func__.__dict__) and plain functions. + """ + method = getattr(test, test._testMethodName) + func = getattr(method, "__func__", method) + return func.__dict__ + + class TablePrinter(object): # Modified from https://github.com/agramian/table-printer, same license as above "Print a list of dicts as a table" @@ -325,9 +335,7 @@ class CustomTextTestResult(result.TestResult): self.stream.writeln("TEST SUITE: %s" % self.suite) self.stream.writeln("Description: %s" % self.getSuiteDescription(test)) try: - name_override = getattr(test, test._testMethodName).__func__.__dict__[ - "test_case_name" - ] + name_override = _get_method_dict(test)["test_case_name"] except: name_override = None self.case = name_override if name_override else self.case @@ -345,7 +353,11 @@ class CustomTextTestResult(result.TestResult): self.results["suites"][self.suite_number] = { "name": self.suite, "class": test.__class__.__name__, - "module": re.compile(".* \((.*)\)").match(str(test)).group(1), + "module": ( + m.group(1) + if (m := re.compile(r".* \((.*)\)").match(str(test))) + else str(test) + ), "description": self.getSuiteDescription(test), "cases": {}, "used_case_names": {}, @@ -380,34 +392,22 @@ class CustomTextTestResult(result.TestResult): if "test_type" in getattr( test, test._testMethodName ).__func__.__dict__ and set([s.lower() for s in self.test_types]) == set( - [ - s.lower() - for s in getattr(test, test._testMethodName).__func__.__dict__[ - "test_type" - ] - ] + [s.lower() for s in _get_method_dict(test)["test_type"]] ): pass else: - getattr(test, test._testMethodName).__func__.__dict__[ - "__unittest_skip_why__" - ] = 'Test run specified to only run tests of type "%s"' % ",".join( - self.test_types + _get_method_dict(test)["__unittest_skip_why__"] = ( + 'Test run specified to only run tests of type "%s"' + % ",".join(self.test_types) ) - getattr(test, test._testMethodName).__func__.__dict__[ - "__unittest_skip__" - ] = True - if "skip_device" in getattr(test, test._testMethodName).__func__.__dict__: - for device in getattr(test, test._testMethodName).__func__.__dict__[ - "skip_device" - ]: + _get_method_dict(test)["__unittest_skip__"] = True + if "skip_device" in _get_method_dict(test): + for device in _get_method_dict(test)["skip_device"]: if self.config and device.lower() in self.config["device_name"].lower(): - getattr(test, test._testMethodName).__func__.__dict__[ - "__unittest_skip_why__" - ] = "Test is marked to be skipped on %s" % device - getattr(test, test._testMethodName).__func__.__dict__[ - "__unittest_skip__" - ] = True + _get_method_dict(test)["__unittest_skip_why__"] = ( + "Test is marked to be skipped on %s" % device + ) + _get_method_dict(test)["__unittest_skip__"] = True break def stopTest(self, test):