FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix pyclass memory layout to prevent silent UB in inherited getter dispatch by 1ndahous3 · Pull Request #7663 · RustPython/RustPython · GitHub

Repository navigation

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension .rs  (3) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
95 changes: 78 additions & 17 deletions crates/derive-impl/src/pyclass.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -318,12 +318,13 @@ pub(crate) fn impl_pyclass_impl(attr: PunctuatedNestedMeta, item: Item) -> Resul
Ok(tokens)
}

/// Validates that when a base class is specified, the struct has the base type as its first field.
/// This ensures proper memory layout for subclassing (required for #[repr(transparent)] to work correctly).
fn validate_base_field(item: &Item, base_path: &syn::Path) -> Result<()> {
/// Validates that when a base class is specified, the struct has the base type as its first
/// *declared* field. Returns a token naming that field (e.g. `_base` or `0` for tuple structs)
/// so the caller can emit a compile-time `offset_of!` assertion, or `None` for non-structs.
fn validate_base_field(item: &Item, base_path: &syn::Path) -> Result<Option<TokenStream>> {
let Item::Struct(item_struct) = item else {
// Only validate structs - enums with base are already an error elsewhere
return Ok(());
return Ok(None);
};

// Get the base type name for error messages
Expand All @@ -347,6 +348,8 @@ fn validate_base_field(item: &Item, base_path: &syn::Path) -> Result<()> {
"#[pyclass] with base = {base_name} requires the first field to be of type {base_name}"
);
}
let ident = first_field.ident.as_ref().map(|id| quote! { #id });
Ok(ident)
}
syn::Fields::Unnamed(fields) => {
let Some(first_field) = fields.unnamed.first() else {
Expand All @@ -361,6 +364,7 @@ fn validate_base_field(item: &Item, base_path: &syn::Path) -> Result<()> {
"#[pyclass] with base = {base_name} requires the first field to be of type {base_name}"
);
}
Ok(Some(quote! { 0 }))
}
syn::Fields::Unit => {
bail_span!(
Expand All @@ -369,8 +373,23 @@ fn validate_base_field(item: &Item, base_path: &syn::Path) -> Result<()> {
);
}
}
}

Ok(())
/// Adds `#[repr(C)]` to a derived pyclass struct when no explicit `#[repr(…)]` is present.
///
/// The inherited getter dispatcher reinterprets the derived object pointer as `*Base`, which
/// is only valid when the base field is at offset 0. Under `#[repr(Rust)]` the compiler may
/// reorder fields to minimise padding, silently displacing the base field. `#[repr(C)]`
/// preserves declaration order, guaranteeing offset 0 for the first field.
fn ensure_repr_c(mut item: Item) -> Item {
let Item::Struct(ref mut s) = item else {
return item;
};
let has_repr = s.attrs.iter().any(|attr| attr.path().is_ident("repr"));
if !has_repr {
s.attrs.push(parse_quote!(#[repr(C)]));
}
item
}

/// Check if a type matches a given path (handles simple cases like `Foo` or `path::to::Foo`)
Expand Down Expand Up @@ -549,19 +568,60 @@ pub(crate) fn impl_pyclass(attr: PunctuatedNestedMeta, item: Item) -> Result<Tok
if matches!(item, syn::Item::Use(_)) {
return Ok(quote!(#item));
}
let (ident, attrs) = pyclass_ident_and_attrs(&item)?;

let fake_ident = Ident::new("pyclass", item.span());
let class_meta = ClassItemMeta::from_nested(ident.clone(), fake_ident, attr.into_iter())?;
let class_name = class_meta.class_name()?;
let module_name = class_meta.module()?;
let base = class_meta.base()?;
let metaclass = class_meta.metaclass()?;
let unhashable = class_meta.unhashable()?;

// Validate that if base is specified, the first field must be of the base type
if let Some(ref base_path) = base {
validate_base_field(&item, base_path)?;
}
let (class_meta, class_name, module_name, base, metaclass, unhashable) = {
let (ident, _) = pyclass_ident_and_attrs(&item)?;
let class_meta = ClassItemMeta::from_nested(ident.clone(), fake_ident, attr.into_iter())?;
let class_name = class_meta.class_name()?;
let module_name = class_meta.module()?;
let base = class_meta.base()?;
let metaclass = class_meta.metaclass()?;
let unhashable = class_meta.unhashable()?;
(
class_meta,
class_name,
module_name,
base,
metaclass,
unhashable,
)
};

// When a base is specified:
// 1. Validate that the first *declared* field has the base type.
// 2. Auto-insert #[repr(C)] so the compiler preserves declaration order,
// keeping the base field at offset 0 as the inherited getter dispatcher requires.
// 3. Emit a compile-time offset_of! assertion as a safety net for structs that
// already carry an explicit repr that does not guarantee offset 0.
let base_field_token = if let Some(ref base_path) = base {
validate_base_field(&item, base_path)?
} else {
None
};

let item = if base.is_some() {
ensure_repr_c(item)
} else {
item
};

let (ident, attrs) = pyclass_ident_and_attrs(&item)?;

let offset_assert = match (&base, &base_field_token) {
(Some(_), Some(field)) => quote! {
const _: () = ::core::assert!(
::core::mem::offset_of!(#ident, #field) == 0,
concat!(
"The base field of `", stringify!(#ident), "` is not at offset 0. \
Add `#[repr(C)]` (or `#[repr(transparent)]`) to the struct so the \
compiler preserves declaration order and inherited getter dispatch \
reads the correct memory."
)
);
},
_ => quote! {},
};

let class_def = generate_class_def(
ident,
Expand Down Expand Up @@ -704,6 +764,7 @@ pub(crate) fn impl_pyclass(attr: PunctuatedNestedMeta, item: Item) -> Result<Tok
let ret = quote! {
#derive_trace
#item
#offset_assert
#maybe_traverse_code
#class_def
#impl_payload
Expand Down
3 changes: 0 additions & 3 deletions crates/stdlib/src/_asyncio.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ pub(crate) mod _asyncio {
#[pyattr]
#[pyclass(name = "Future", module = "_asyncio", traverse)]
#[derive(Debug, PyPayload)]
#[repr(C)] // Required for inheritance - ensures base field is at offset 0 in subclasses
struct PyFuture {
fut_loop: PyRwLock<Option<PyObjectRef>>,
fut_callback0: PyRwLock<Option<PyObjectRef>>,
Expand Down Expand Up @@ -1098,9 +1097,7 @@ pub(crate) mod _asyncio {
#[pyattr]
#[pyclass(name = "Task", module = "_asyncio", base = PyFuture, traverse)]
#[derive(Debug)]
#[repr(C)]
struct PyTask {
// Base class (must be first field for inheritance)
base: PyFuture,
// Task-specific fields
task_coro: PyRwLock<Option<PyObjectRef>>,
Expand Down
3 changes: 0 additions & 3 deletions crates/vm/src/builtins/builtin_func.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,6 @@ impl PyNativeFunction {
}

// PyCMethodObject in CPython
// repr(C) ensures `func` is at offset 0, allowing safe cast from PyNativeMethod to PyNativeFunction
#[repr(C)]
#[pyclass(name = "builtin_function_or_method", module = false, base = PyNativeFunction, ctx = "builtin_function_or_method_type")]
pub struct PyNativeMethod {
pub(crate) func: PyNativeFunction,
Expand All @@ -211,7 +209,6 @@ pub struct PyNativeMethod {

// All Python-visible behavior (getters, slots) is registered by PyNativeFunction::extend_class.
// PyNativeMethod only extends the Rust-side struct with the defining class reference.
// The func field at offset 0 (#[repr(C)]) allows NativeFunctionOrMethod to read it safely.
#[pyclass(flags(HAS_DICT, HAS_WEAKREF, DISALLOW_INSTANTIATION))]
impl PyNativeMethod {}

Expand Down
Loading

Back | FazBrowse Home | New Git URL