`PyFrame::builtins` is not guaranteed to be a `dict`

[davidhewitt]

In #5967 we added PyFrame::builtins, which uses cast_into_unchecked() to assume the returned object is a dict.

    #[cfg(all(Py_3_11, not(Py_LIMITED_API)))]
    fn builtins(&self) -> Bound<'py, PyDict> {
        // SAFETY:
        // - we're attached to the interpreter
        // - `self` is a `PyFrameObject`
        // - `PyFrame_GetBuiltins` returns an owned reference
        // - the result can not be null
        // - the result is a dict object
        unsafe {
            ffi::PyFrame_GetBuiltins(self.as_ptr().cast())
                .assume_owned_unchecked(self.py())
                .cast_into_unchecked()
        }
    }

Unfortunately it's possible to set frame builtins to an arbitrary object, as per this PoC:

use pyo3::prelude::*;
use pyo3::types::{PyAnyMethods, PyDict, PyFrame, PyFrameMethods};

fn main() {
    pyo3::Python::initialize();
    Python::attach(|py| -> PyResult<()> {
        let sys = py.import("sys")?;
        let globals = PyDict::new(py);
        globals.set_item("getframe", sys.getattr("_getframe")?)?;
        globals.set_item("__builtins__", py.eval(c"object()", None, None)?)?;
        py.run(c"frame = getframe()", Some(&globals), None)?;

        let frame_obj = globals.get_item("frame")?.expect("frame set");
        let frame: Bound<'_, PyFrame> = frame_obj.cast_into()?;

        // This assertion fails
        assert!(frame.builtins().is_instance_of::<PyDict>());
        Ok(())
    })
    .unwrap();
}

... I suppose we need to re-audit the frame methods and work out which of these need checked casts? I wonder if there are similar gaps on other cast_into_unchecked callsites around the codebase, I think generally we've used it on methods where the interpreter is constructing new objects so we are certain what's being produced.

I can't decide whether it would be nicer to:

• Keep the PyDict return type, and panic if the type is not as expected.
• Return PyResult<Bound<'_, PyDict>> and force users to handle the unlikely error.
• Return Bound<'_, PyAny> and force users to call .get_item etc

My instinct is that the panic here might be appropriate, I don't think there's a reasonable case where frame builtins can be a non-dict in a functionally useful way. (... maybe at worst, some mock object in unit tests?)

cc @Icxolu

(Credit to Codex security scanning for this discovery.)

[Icxolu]

Ouch, this is unfortunate. Given that it quite convoluted to create such an "invalid" frame and that CPython actually documents that this should be a dict here, I'd also say that panicking is the right move here to ensure correctness even with such hostile code.