Skip to content

Use active builtins consistently in descriptor and method reductions #9057

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Oct 12, 2026
Merged

Use active builtins consistently in descriptor and method reductions #9057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/vm/src/builtins/builtin_func.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ impl PyNativeFunction {
.as_ref()
.filter(|bound| !bound.class().is_subtype(vm.ctx.types.module_type))
{
let getattr = vm.builtins.get_attr("getattr", vm)?;
let getattr = vm.eval_get_builtin(vm.ctx.intern_str("getattr"))?;
Ok(vm
.new_tuple((getattr, (bound.clone(), zelf.value.name)))
.into())
Expand Down
16 changes: 9 additions & 7 deletions crates/vm/src/builtins/descriptor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,12 @@ impl Py<PyMethodDescriptor> {
fn __reduce__(
&self,
vm: &VirtualMachine,
) -> (Option<PyObjectRef>, (Option<PyObjectRef>, &'static str)) {
let builtins_getattr = vm.builtins.get_attr("getattr", vm).ok();
let classname = vm.builtins.get_attr(&self.common.typ.__name__(vm), vm).ok();
(builtins_getattr, (classname, self.method.name))
) -> PyResult<(PyObjectRef, (PyTypeRef, &'static str))> {
let builtins_getattr = vm.eval_get_builtin(vm.ctx.intern_str("getattr"))?;
Ok((
builtins_getattr,
(self.common.typ.to_owned(), self.method.name),
))
}
}

Expand Down Expand Up @@ -851,7 +853,7 @@ impl Py<PyMemberDescriptor> {

#[pymethod]
fn __reduce__(&self, vm: &VirtualMachine) -> PyResult {
let builtins_getattr = vm.builtins.get_attr("getattr", vm)?;
let builtins_getattr = vm.eval_get_builtin(vm.ctx.intern_str("getattr"))?;
Ok(vm
.ctx
.new_tuple(vec![
Expand Down Expand Up @@ -2064,7 +2066,7 @@ impl Callable for PyWrapper {
impl Py<PyWrapper> {
#[pymethod]
fn __reduce__(&self, vm: &VirtualMachine) -> PyResult {
let getattr = vm.builtins.get_attr("getattr", vm)?;
let getattr = vm.eval_get_builtin(vm.ctx.intern_str("getattr"))?;

@youknowone youknowone Oct 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tell me why this is important. and also this pattern is looking like need to be applied more places. if it does, patch and include here

@youknowdot youknowdot Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is observable compatibility of the reconstruction recipe. With a custom active builtins mapping, CPython returns that mapping’s exact getattr object from reduce, without calling it; if the entry is absent, reduction raises AttributeError("getattr"). Reading vm.builtins instead returns a different callable and hides that lookup failure. This is not a sandbox-security claim.

You are right about the other sites. Exact CPython 3.14.8 source and bounded reference/native checks identify the same issue in method/member descriptors, bound native methods, Python bound methods, and method wrappers. I am including those fixes here, with stored defining-type handling and name lookup order preserved, and expanding the existing parameterized Python regression. Module-bound functions still return a name. Classmethod/getset descriptors and pickle’s separately cached getattr have different contracts and are not being changed.

@youknowdot youknowdot Oct 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Included in 4944d19: all five remaining reducer sites now use active builtins; method descriptors retain their actual defining type; Python bound methods keep name lookup first. The helper now performs one generic lookup for dictionary subclasses, preserving inherited missing without an extra fallback after overridden getitem. The original wrapper assertions remain covered in the expanded table.

Exact CPython 3.14.7/3.14.8 reference matrices and the native candidate agree, including callback counts and exception order. The full pickle snippet, both snippet pytest entries, four unchanged canonical method/iterator tests, normal hooks, and VM/CAPI Clippy pass. Two pre-existing C-method pickle expected failures remain unchanged. Module-bound functions, classmethod/getset descriptors, and pickle’s cached getattr retain their distinct contracts. New-head CI is running.

let args = vm.ctx.new_tuple(vec![
self.typ.to_owned().into(),
self.name.to_owned().into(),
Expand Down Expand Up @@ -2181,7 +2183,7 @@ impl Py<PyMethodWrapper> {

#[pymethod]
fn __reduce__(zelf: PyRef<PyMethodWrapper>, vm: &VirtualMachine) -> PyResult {
let builtins_getattr = vm.builtins.get_attr("getattr", vm)?;
let builtins_getattr = vm.eval_get_builtin(vm.ctx.intern_str("getattr"))?;
Ok(vm
.ctx
.new_tuple(vec![
Expand Down
4 changes: 2 additions & 2 deletions crates/vm/src/builtins/function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1470,9 +1470,9 @@ impl Py<PyBoundMethod> {
&self,
vm: &VirtualMachine,
) -> PyResult<(PyObjectRef, (PyObjectRef, PyObjectRef))> {
let builtins_getattr = vm.builtins.get_attr("getattr", vm)?;
let func_self = self.object.clone();
let func_name = self.function.get_attr("__name__", vm)?;
let builtins_getattr = vm.eval_get_builtin(vm.ctx.intern_str("getattr"))?;
let func_self = self.object.clone();
Ok((builtins_getattr, (func_self, func_name)))
}

Expand Down
2 changes: 1 addition & 1 deletion crates/vm/src/vm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3170,7 +3170,7 @@ impl VirtualMachine {
pub fn eval_get_builtin(&self, name: &'static PyStrInterned) -> PyResult {
let builtins =
crate::frame::current_builtins().unwrap_or_else(|| self.builtins.dict().into());
if let Some(dict) = builtins.downcast_ref::<PyDict>() {
if let Some(dict) = builtins.downcast_ref_if_exact::<PyDict>(self) {
match dict.get_item_opt(name, self)? {
Some(value) => Ok(value),
None => Err(self.new_attribute_error(name.to_string())),
Expand Down
81 changes: 81 additions & 0 deletions extra_tests/snippets/stdlib_pickle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import _pickle
import io
import sys
from types import MappingProxyType, MethodType

from testutils import assert_raises

Expand Down Expand Up @@ -35,3 +36,83 @@
else:
expected = f"{name} exceeds system's maximum size of {sys.maxsize} bytes"
assert str(caught.exception) == expected


# Reduction returns getattr from the active frame's builtins without calling it.

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Remove prohibited test-logic changes

This added regression block introduces new test data, execution logic, and assertions in a test file, while the repository permits test-file edits only for adding or removing expected-failure decorators; the test additions therefore need to be removed unless the maintainers revise that policy first.

AGENTS.md reference: AGENTS.md:L275-L279

Useful? React with 👍 / 👎.

class ReductionOwner:
__slots__ = ("slot",)

def method(self):
pass


owner = ReductionOwner()
items = []
replacement_getattr = object()
for descriptor, receiver, name in (
(object.__str__, object, "__str__"),
(str.join, str, "join"),
(io.BytesIO.getvalue, io.BytesIO, "getvalue"),
(ReductionOwner.slot, ReductionOwner, "slot"),
(owner.__str__, owner, "__str__"),
(items.append, items, "append"),
(dict.fromkeys, dict, "fromkeys"),
(owner.method, owner, "method"),
):
for builtins_type in (dict, MappingProxyType):
namespace = {
"__builtins__": builtins_type({"getattr": replacement_getattr}),
"descriptor": descriptor,
}
exec("reduced = descriptor.__reduce__()", namespace)
assert namespace["reduced"] == (replacement_getattr, (receiver, name))

namespace["__builtins__"] = builtins_type({})
with assert_raises(AttributeError) as caught:
exec("descriptor.__reduce__()", namespace)
assert caught.exception.args == ("getattr",)

# Module-bound functions reduce to a name without looking up getattr.
namespace = {"__builtins__": {}, "function": len}
exec("reduced = function.__reduce__()", namespace)
assert namespace["reduced"] == "len"


class MissingBuiltin(dict):
def __getitem__(self, key):
raise KeyError(key)

def __missing__(self, key):
raise AssertionError("must not retry __missing__ after custom __getitem__")


for descriptor, name in (
(object.__str__, "getattr"),
(iter([1]), "iter"),
(reversed([1]), "reversed"),
):
namespace = {"__builtins__": MissingBuiltin(), "descriptor": descriptor}
with assert_raises(AttributeError) as caught:
exec("descriptor.__reduce__()", namespace)
assert caught.exception.args == (name,)


name_error = RuntimeError("function name first")


class NamedCallable:
def __call__(self, *args):
pass

@property
def __name__(self):
raise name_error


namespace = {
"__builtins__": {},
"method": MethodType(NamedCallable(), owner),
}
with assert_raises(RuntimeError) as caught:
exec("method.__reduce__()", namespace)
assert caught.exception is name_error
Loading