Fix miri UB: use transmute_copy for function pointer identity checks - #8432
Conversation
Nightly miri now flags `fn_ptr as usize` and direct `fn_ptr == fn_ptr` as UB because both paths go through `FnPtr::addr()`, which attempts to dereference a function pointer's provenance — function items have no backing allocation in miri's model. Add `fn_addr<T>(f: T) -> usize` that uses `transmute_copy` to read the address as plain integer bytes without triggering provenance checks. Replace all `f as usize` slot comparison patterns across the codebase with `fn_addr(f)`. Also add `-Zmiri-permissive-provenance` to CI MIRIFLAGS as a safety net for any remaining integer-pointer round-trips elsewhere. Assisted-by: Claude
📝 WalkthroughWalkthroughThe PR adds a shared ChangesFunction address provenance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses new Miri Undefined Behavior reports caused by function-pointer identity comparisons in the VM by introducing a helper that extracts a function pointer’s raw address without invoking FnPtr::addr() and then using it at various slot-comparison call sites.
Changes:
- Add
fn_addrhelper (viatransmute_copy) for function-pointer identity checks under Miri. - Replace a set of
fn_ptr as usize-style slot/function identity comparisons withfn_addr(...). - Update the Miri CI job flags to include
-Zmiri-permissive-provenance.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/vm/src/types/slot.rs | Adds fn_addr utility and updates slot function comparisons to use it. |
| crates/vm/src/types/slot_defs.rs | Uses fn_addr for init slot “slot defined” checks. |
| crates/vm/src/class.rs | Switches slot function identity checks (e.g., __hash__, __new__ inheritance) to fn_addr. |
| crates/vm/src/frame.rs | Updates specialization guards to compare slot functions via fn_addr. |
| crates/vm/src/builtins/type.rs | Uses fn_addr for tp_new safety/identity checks. |
| crates/vm/src/builtins/object.rs | Compares tp_init via fn_addr rather than integer casts. |
| crates/vm/src/builtins/set.rs | Updates frozenset init identity check to use fn_addr. |
| crates/vm/src/stdlib/_thread.rs | Updates Local init detection to use fn_addr. |
| crates/vm/src/vm/method.rs | Uses fn_addr for default getattro detection. |
| crates/vm/src/vm/vm_ops.rs | Uses fn_addr for number-slot address comparisons in binary/ternary dispatch. |
| .github/workflows/ci.yaml | Adds -Zmiri-permissive-provenance to MIRIFLAGS for the Miri CI job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # miri-permissive-provenance because function pointer identity checks (slot comparisons) | ||
| # cast fn pointers to usize, which strips provenance — this is the standard pattern for | ||
| # fn pointer comparison in Rust and not a soundness issue | ||
| MIRIFLAGS: "-Zmiri-ignore-leaks -Zmiri-permissive-provenance" |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/types/slot.rs`:
- Around line 2174-2194: Remove the `fn_addr` helper and replace its direct
function-pointer comparison sites with `core::ptr::fn_addr_eq`, using negation
where the existing logic checks inequality. Preserve the existing `fn_addr_eq`
usage in `vm_ops.rs`; for intermediate `Option<usize>` values, derive the value
from `fn_addr_eq` rather than storing `transmute_copy` addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 83cc0a0a-2699-4345-9373-16dabd92b5c4
📒 Files selected for processing (11)
.github/workflows/ci.yamlcrates/vm/src/builtins/object.rscrates/vm/src/builtins/set.rscrates/vm/src/builtins/type.rscrates/vm/src/class.rscrates/vm/src/frame.rscrates/vm/src/stdlib/_thread.rscrates/vm/src/types/slot.rscrates/vm/src/types/slot_defs.rscrates/vm/src/vm/method.rscrates/vm/src/vm/vm_ops.rs
|
|
||
| /// Extract the raw address of a function pointer as `usize` without | ||
| /// triggering miri's "pointer not dereferenceable" UB. | ||
| /// | ||
| /// The standard `fn_ptr as usize` cast goes through `FnPtr::addr()` | ||
| /// which attempts to dereference the function pointer's provenance — | ||
| /// miri considers this UB for function items. `transmute_copy` bypasses | ||
| /// that path and reads the address as plain integer bytes. | ||
| /// | ||
| /// The result is suitable for identity comparison only: two function | ||
| /// pointers with the same address are the same function. The converse | ||
| /// is not always guaranteed (the compiler may merge identical function | ||
| /// bodies), but this matches CPython's slot comparison semantics. | ||
| #[inline(always)] | ||
| pub(crate) fn fn_addr<T: Copy>(f: T) -> usize { | ||
| assert!( | ||
| core::mem::size_of::<T>() == core::mem::size_of::<usize>(), | ||
| "fn_addr: T must be pointer-sized" | ||
| ); | ||
| unsafe { core::mem::transmute_copy::<T, usize>(&f) } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What Rust version stabilized std::ptr::fn_addr_eq, and is it available in core::ptr?
💡 Result:
The function core::ptr::fn_addr_eq was stabilized in Rust version 1.85.0 [1][2][3]. Yes, it is available in core::ptr [2][3][4], and it is also re-exported via std::ptr [5][6].
Citations:
- 1: https://releases.rs/docs/1.85.0/
- 2: https://docs.rust-lang.org/core/ptr/fn.fn_addr_eq.html
- 3: https://doc.rust-lang.org/stable/core/ptr/fn.fn_addr_eq.html
- 4: https://doc.rust-lang.org/core/ptr/fn.fn_addr_eq.html
- 5: https://doc.rust-lang.org/stable/std/ptr/fn.fn_addr_eq.html
- 6: https://doc.rust-lang.org/std/ptr/fn.fn_addr_eq.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching slot/vm_ops/method.rs =="
fd -a 'slot\.rs$|vm_ops\.rs$|method\.rs$|slot_defs\.rs$' crates/vm/src | sed 's#^\./##'
echo
echo "== rust toolchain files =="
git ls-files | rg '(^|/)(rust-toolchain(\.toml)?|Cargo\.toml|Cargo\.lock)$' || true
if [ -f rust-toolchain ]; then cat -n rust-toolchain; fi
if [ -f rust-toolchain.toml ]; then cat -n rust-toolchain.toml; fi
if [ -f Cargo.toml ]; then rg -n 'edition|rust-version|rustc' Cargo.toml || true; fi
echo
echo "== fn_addr and fn_addr_eq occurrences =="
rg -n -C 2 '\bfn_addr\b|fn_addr_eq|transmute_copy' crates || true
echo
echo "== relevant slot.rs sections around fn_addr and call sites =="
python3 - <<'PY'
from pathlib import Path
p = Path("crates/vm/src/types/slot.rs")
lines = p.read_text().splitlines()
for i in [1, 940, 990, 2150, 2174]:
start=max(1,i-5); end=min(len(lines),i+15)
print(f"\n--- {p}:{start}-{end} ---")
for n in range(start,end+1):
print(f"{n:5}: {lines[n-1]}")
PY
echo
echo "== relevant vm_ops.rs section around existing fn_addr_eq =="
p="crates/vm/src/vm/vm_ops.rs"
lines=p.read_text().splitlines()
for start,end in [(330,375),(980,1045),(1060,1115)]:
print(f"\n--- {p}:{start}-{end} ---")
for n in range(start,end+1):
if n <= len(lines):
print(f"{n:5}: {lines[n-1]}")
echo
echo "== relevant slot_defs.rs copyslot_if_none section =="
p="crates/vm/src/types/slot_defs.rs"
if p.exists():
lines=p.read_text().splitlines()
rg -n "copyslot_if_none|fn_addr|fn_addr_eq" $p || true
for start,end in [(1,80), (200,300), (300,400)]:
matches=[i for i,l in enumerate(lines,1) if 'copyslot_if_none' in l or 'fn_addr' in l]
if any(start-m <= s <= end+m for s in matches):
print(f"\n--- {p}:{start}-{end} ---")
for n in range(start,end+1):
print(f"{n:5}: {lines[n-1]}")
else:
fd -a 'slot_defs\.rs$' .Repository: RustPython/RustPython
Length of output: 20995
Replace fn_addr with core::ptr::fn_addr_eq at the comparison sites.
core::ptr::fn_addr_eq is available in the configured Rust toolchain and gives the intended safe function-pointer identity comparison. Replace direct pair comparisons such as fn_addr(set) == fn_addr(del) with core::ptr::fn_addr_eq(set, del) or !core::ptr::fn_addr_eq(...). Keep the existing fn_addr_eq calls in crates/vm/src/vm/vm_ops.rs; where an intermediate Option<usize> is stored, derive it from fn_addr_eq instead of storing transmute_copy output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/vm/src/types/slot.rs` around lines 2174 - 2194, Remove the `fn_addr`
helper and replace its direct function-pointer comparison sites with
`core::ptr::fn_addr_eq`, using negation where the existing logic checks
inequality. Preserve the existing `fn_addr_eq` usage in `vm_ops.rs`; for
intermediate `Option<usize>` values, derive the value from `fn_addr_eq` rather
than storing `transmute_copy` addresses.
Problem
Nightly miri now flags function pointer identity checks as UB. Both
fn_ptr as usizeand directfn_ptr == fn_ptrinternally callFnPtr::addr(), which attempts to dereference the function pointer's provenance. Since function items have no backing allocation in miri's model, this triggers:Solution
Add
fn_addr<T>(f: T) -> usizeincrate::typesthat usestransmute_copyto read the function pointer's address as plain integer bytes, bypassingFnPtr::addr()entirely:Replace all
f as usizefunction pointer comparison patterns across the codebase (11 files, ~20 call sites) withfn_addr(f).Also add
-Zmiri-permissive-provenanceto CI MIRIFLAGS as a safety net for any remaining integer-pointer round-trips elsewhere.Files changed
types/slot.rs— addfn_addrutility, update slot comparisonstypes/slot_defs.rs—initslot SLOTDEFINED checkclass.rs—hash_not_implementedand__new__slot checksframe.rs—getattro,setattro,tp_new,tp_allocspecialization guardsbuiltins/type.rs—tp_newsafety check,new_wrapperdetectionbuiltins/object.rs—tp_initvsobject.__init__builtins/set.rs— frozensettp_initcheckstdlib/_thread.rs—Localcustom init detectionvm/method.rs—getattrodefault checkvm/vm_ops.rs— binary/ternary number op slot identity.github/workflows/ci.yaml— add-Zmiri-permissive-provenanceVerification
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests