From 6ef5309417f185c5a440bbf26a7505ab7bbf15e7 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 27 Aug 2026 12:05:05 +0000 Subject: [PATCH] r2il: resolve the Custom(n) non-determinism at its source, and guard the row Two open rows in probes/win32-census/EXECUTION-NOTES.md closed together, because they turned out to be one defect: `Ram`/`Custom` aliasing and persisted-row safety. ROOT CAUSE, which the notes had misdiagnosed. The file recorded the symptom ("Custom(n) is non-deterministic") and left the mechanism open. There are two producers of `SpaceId::Custom` and only one was broken: * `Disassembler::translate_space` hashes the space NAME. Deterministic. Cleared by reading it, not by assuming it. * `space_from_index` (three impls) takes a LOAD/STORE's input-0 constant. Ghidra p-code encodes the target space there as the host `AddrSpace*` POINTER, not an index -- so `spaces.get(ptr as usize)` always missed and the fallback truncated the pointer into `Custom(ptr as u32)`. That is why one instruction lifted to Custom(1062180976), Custom(2279590000) and Custom(1968052336) across three runs: ASLR, bottled into a Serialize-derived field. Both an unreproducible lift and an unreproducible persisted row. FIX. The handle is dropped rather than bottled: a new payload-free `SpaceId::Unresolved`. Two runs now lift identically because nothing process-specific reaches the IR. All unresolved handles compare equal, which is an over-match and never an under-match -- we do not know whether two are the same space, and claiming they differ on the strength of a pointer is the stronger lie. `r2conc` fails closed (`ConcError::UnresolvedSpace`) rather than guessing RAM. ALIASING. `AddressSpace::aliases` declares "same memory, second identity" without merging the ids: a space that is a window onto RAM keeps its own SpaceId (the lift stays faithful to the SLEIGH spec) while naming where its bytes live. Reserve, don't claim. Additive and serde-default, so old rows read unchanged. No lifter populates it yet. A DESIGN CORRECTION MID-IMPLEMENTATION, recorded because the measurement caused it. The first cut made `Serialize` REFUSE `Unresolved`. That broke 24 of 124 r2sleigh-plugin tests on ordinary x86 input -- baselined against a stash first, so caused rather than pre-existing. Cause: serde here also backs in-process cache keys (`hash_json_value(&blocks)`) and the plugin's diagnostic JSON, not only persistence. The guarantee moved to the write path: `Unresolved` serializes as the visible token "Unresolved" (never a plausible number), and `Varnode::is_persistable()` is the guard a write path calls. Precisely: "cannot be written unnoticed", not "cannot be written" -- r2il has no row-persist entry point yet, so the guard is available rather than wired. The failure also measured something: those 24 tests passed before because the pointer serialized fine, i.e. they had been hashing ASLR'd pointers into cache keys all along. Harmless in-process, and proof the unresolved path is hit constantly on real x86 rather than in a corner. Falsifiers, each verified red with the fix reverted, and the disable named in the test's own doc comment: * unresolvable_space_pointers_from_three_runs_convert_identically -- runs the REAL `convert_varnode` fallback on the three measured pointer values, with an anti-vacuity assert that they genuinely miss the space map. * a_resolvable_space_index_still_resolves -- the silence twin; without it a convert that returned Unresolved for everything would pass the above. * an_unresolved_space_persists_as_a_self_describing_token * is_persistable_refuses_only_the_unreproducible_varnode -- two-sided. * an_aliased_space_names_ram_without_becoming_ram * is_unresolved_discriminates A first version of the determinism test compared `Unresolved` to itself and never touched the lifter. Caught and replaced before it landed. Also fixes a stale notes row: `R2ILOp = 464 bytes` still read "open prerequisite" three commits after Move A closed it. Gates: cargo test --workspace green (incl. r2sleigh-plugin 124/124), clippy --workspace --all-targets clean, fmt clean. --- Cargo.lock | 1 + crates/r2conc/src/lib.rs | 9 ++ crates/r2il/src/serialize.rs | 4 + crates/r2il/src/space.rs | 203 ++++++++++++++++++++++++- crates/r2il/src/varnode.rs | 17 +++ crates/r2sleigh-lift/Cargo.toml | 3 + crates/r2sleigh-lift/src/context.rs | 2 + crates/r2sleigh-lift/src/disasm.rs | 10 +- crates/r2sleigh-lift/src/pcode.rs | 76 ++++++++- crates/r2sleigh-lift/src/translate.rs | 2 +- crates/r2ssa/src/block.rs | 2 + crates/r2ssa/src/naming.rs | 1 + probes/win32-census/EXECUTION-NOTES.md | 77 +++++++++- r2plugin/src/lib.rs | 8 + 14 files changed, 406 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2c6b238d8..054150f0b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -527,6 +527,7 @@ version = "0.1.0" dependencies = [ "libsla", "r2il", + "serde_json", "sleigh-config", "thiserror 2.0.17", ] diff --git a/crates/r2conc/src/lib.rs b/crates/r2conc/src/lib.rs index 2de0938ad..52c099905 100644 --- a/crates/r2conc/src/lib.rs +++ b/crates/r2conc/src/lib.rs @@ -88,6 +88,11 @@ pub enum ConcError { /// A `Custom(n)` space with no slab registered. #[error("no slab registered for custom space {0}")] UnknownSpace(u32), + /// The lift could not resolve this space handle, so execution cannot + /// know which slab it means. See [`r2il::SpaceId::Unresolved`] — this is + /// deliberately an error rather than a fallback to RAM. + #[error("space was unresolved at lift time — no slab can be chosen")] + UnresolvedSpace, /// Unsigned or signed division / remainder by zero. #[error("division by zero")] DivByZero, @@ -216,6 +221,10 @@ impl<'a> SlabState<'a> { .find(|(i, _)| *i == n) .map(|(_, s)| &mut **s) .ok_or(ConcError::UnknownSpace(n)), + // Fail closed. An unresolved handle names no space, so there is + // no slab that could be the right one -- and guessing RAM would + // silently corrupt whichever slab it actually meant. + SpaceId::Unresolved => Err(ConcError::UnresolvedSpace), SpaceId::Const => unreachable!("Const handled before slab lookup"), } } diff --git a/crates/r2il/src/serialize.rs b/crates/r2il/src/serialize.rs index 41a485d0a..1f4b0a6e5 100644 --- a/crates/r2il/src/serialize.rs +++ b/crates/r2il/src/serialize.rs @@ -225,6 +225,8 @@ impl From for AddressSpace { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + // Pre-alias rows carry no alias information. + aliases: None, } } } @@ -253,6 +255,8 @@ impl From for AddressSpace { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + // Pre-alias rows carry no alias information. + aliases: None, } } } diff --git a/crates/r2il/src/space.rs b/crates/r2il/src/space.rs index 2ae77ac3b..ac1c1fca0 100644 --- a/crates/r2il/src/space.rs +++ b/crates/r2il/src/space.rs @@ -18,8 +18,59 @@ pub enum SpaceId { Unique, /// Constant/immediate values Const, - /// Architecture-specific custom space + /// Architecture-specific custom space, identified by a **stable, + /// name-derived** id. + /// + /// **Invariant (load-bearing, see [`SpaceId::Unresolved`]):** the `u32` + /// is a pure function of the space's NAME. It is never an array index, + /// never a host pointer, and never anything else that varies between + /// processes. Two runs over the same binary must produce byte-identical + /// `Custom` ids, because this value is serialized into persisted rows. Custom(u32), + /// A space handle the lifter could **not** resolve to a known space. + /// + /// Deliberately payload-free. The situation that produces it is a + /// LOAD/STORE whose input-0 constant is a raw host `AddrSpace*` pointer + /// rather than a space index (Ghidra p-code encodes the space that way), + /// so the only value available to carry here is run-local: it differs on + /// every process and is meaningless after the process exits. + /// + /// Carrying that value was the defect. Three separate runs over the same + /// x86-64 binary produced `Custom(1062180976)`, `Custom(2279590000)` and + /// `Custom(1968052336)` for the same instruction — an unreproducible lift + /// and, worse, an unreproducible *persisted row*. + /// + /// So the handle is dropped rather than bottled. Consequences, all of + /// them intentional: + /// + /// * **Reproducible.** Two runs lift to the same IR, because nothing + /// process-specific reaches it. + /// * **Not a scan key.** Every unresolved space compares equal. That is + /// an over-match, never an under-match — we genuinely do not know + /// whether two unresolved handles name the same space, and claiming + /// they differ on the strength of a pointer is the stronger lie. + /// * **Visibly unpersistable.** It serializes as the self-describing + /// token `"Unresolved"` — never as a number that reads like a real + /// space id. Any row carrying it announces that it cannot be + /// reproduced, and [`Varnode::is_persistable`] is the guard a write + /// path calls to refuse it. + /// + /// This is deliberately weaker than making `Serialize` fail. That was + /// tried first and was wrong: serde here also backs in-process cache + /// keys and the r2 plugin's diagnostic JSON, and a blanket refusal + /// broke 24 plugin tests on ordinary x86 input. Which incidentally + /// measured how common this path is — those fixtures had been hashing + /// ASLR'd pointers into cache keys all along. The guarantee is + /// therefore "cannot be written *unnoticed*", not "cannot be written"; + /// the refusal lives at the write path, where persistence actually + /// happens. + /// * **Fails closed at execution.** `r2conc` returns an error rather than + /// guessing a slab. + /// + /// Resolving these properly — mapping the pointer back through libsla to + /// a named space — is the follow-up. Until then the lift is honest about + /// not knowing, instead of confidently wrong. + Unresolved, } impl SpaceId { @@ -44,6 +95,17 @@ impl SpaceId { } } +impl SpaceId { + /// True for a space handle the lifter could not resolve. + /// + /// Callers that key a scan, a cache, or a slab lookup on `SpaceId` must + /// check this first: an unresolved handle is not a space, it is the + /// absence of one. + pub fn is_unresolved(&self) -> bool { + matches!(self, SpaceId::Unresolved) + } +} + impl std::fmt::Display for SpaceId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -52,6 +114,7 @@ impl std::fmt::Display for SpaceId { SpaceId::Unique => write!(f, "uniq"), SpaceId::Const => write!(f, "const"), SpaceId::Custom(id) => write!(f, "space{}", id), + SpaceId::Unresolved => write!(f, "unresolved"), } } } @@ -87,6 +150,18 @@ pub struct AddressSpace { /// Optional segment identifier. #[serde(default)] pub segment_id: Option, + /// The space this one *aliases*, if any — same underlying memory reached + /// through a second identity. + /// + /// Reserve, don't claim: an architecture space that is really a window + /// onto RAM keeps its own [`SpaceId`] (so the lift stays faithful to what + /// the SLEIGH spec said) while declaring that reads and writes land in + /// RAM's bytes. A consumer scanning for RAM can follow the alias; nothing + /// merges the two ids, and no lift is rewritten. + /// + /// `None` means "not known to alias anything", never "known not to". + #[serde(default)] + pub aliases: Option, } impl AddressSpace { @@ -104,6 +179,7 @@ impl AddressSpace { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + aliases: None, } } @@ -121,6 +197,7 @@ impl AddressSpace { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + aliases: None, } } @@ -138,6 +215,7 @@ impl AddressSpace { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + aliases: None, } } @@ -155,6 +233,7 @@ impl AddressSpace { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + aliases: None, } } @@ -172,6 +251,7 @@ impl AddressSpace { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + aliases: None, } } } @@ -181,6 +261,127 @@ mod tests { use super::{AddressSpace, SpaceId}; use crate::Endianness; + // ---- Ram/Custom aliasing + persisted-row safety ------------------- + // + // Both rows of `probes/win32-census/EXECUTION-NOTES.md`'s open table. + // Each test below was verified to FAIL with the fix reverted; the + // disable is named in the test's own comment. + + /// Persisted-row safety, restated for the token contract: an + /// unresolvable space must be VISIBLE in any row it reaches, never + /// disguised as a plausible space id. + /// + /// DISABLE: give `Unresolved` a `u32` payload carrying the raw pointer + /// and the first assert fails — the row goes back to looking like a + /// real space. + #[test] + fn an_unresolved_space_persists_as_a_self_describing_token() { + // It says what it is. A reader can detect and reject it. + assert_eq!( + serde_json::to_string(&SpaceId::Unresolved).unwrap(), + "\"Unresolved\"" + ); + + // Anti-vacuity: it is distinguishable from every real space, and in + // particular does not render as a number the way the old + // `Custom()` did. + let token = serde_json::to_string(&SpaceId::Unresolved).unwrap(); + assert!(!token.contains("Custom")); + assert!(!token.chars().any(|c| c.is_ascii_digit())); + + // The other five keep their exact wire format, so existing rows + // round-trip untouched. + assert_eq!(serde_json::to_string(&SpaceId::Ram).unwrap(), "\"Ram\""); + assert_eq!( + serde_json::to_string(&SpaceId::Custom(7)).unwrap(), + "{\"Custom\":7}" + ); + for sid in [ + SpaceId::Ram, + SpaceId::Register, + SpaceId::Unique, + SpaceId::Const, + SpaceId::Custom(4_242), + SpaceId::Unresolved, + ] { + let json = serde_json::to_string(&sid).unwrap(); + let back: SpaceId = serde_json::from_str(&json).unwrap(); + assert_eq!(back, sid); + } + } + + /// The guard a write path calls. Two-sided: it must refuse the + /// unreproducible varnode AND admit every reproducible one, or it + /// carries no information. + /// + /// DISABLE: `is_persistable` returning `true` unconditionally fails the + /// first assert; returning `false` unconditionally fails the loop. + #[test] + fn is_persistable_refuses_only_the_unreproducible_varnode() { + use crate::Varnode; + + assert!(!Varnode::new(SpaceId::Unresolved, 0x1000, 8).is_persistable()); + + for sid in [ + SpaceId::Ram, + SpaceId::Register, + SpaceId::Unique, + SpaceId::Const, + SpaceId::Custom(0x1234), + ] { + assert!( + Varnode::new(sid, 0x1000, 8).is_persistable(), + "{sid} is reproducible and must be persistable" + ); + } + } + + /// `is_unresolved` discriminates — it neither fires on everything nor + /// stays silent on everything. + #[test] + fn is_unresolved_discriminates() { + assert!(SpaceId::Unresolved.is_unresolved()); + for sid in [ + SpaceId::Ram, + SpaceId::Register, + SpaceId::Unique, + SpaceId::Const, + SpaceId::Custom(0), + ] { + assert!(!sid.is_unresolved(), "{sid} must not read as unresolved"); + } + } + + /// Ram/Custom aliasing: reserve the identity, don't claim the memory. + /// + /// A space that is really a window onto RAM keeps its own id (the lift + /// stays faithful to the SLEIGH spec) while declaring where its bytes + /// live. Nothing merges the two. + /// + /// DISABLE: drop the `aliases` field and this stops compiling. + #[test] + fn an_aliased_space_names_ram_without_becoming_ram() { + let mut window = AddressSpace::new(SpaceId::Custom(0x1234), "iomem", 8); + window.aliases = Some(SpaceId::Ram); + + // Reserved: still its own identity. + assert_ne!(window.id, SpaceId::Ram); + // Not claimed: the bytes are RAM's. + assert_eq!(window.aliases, Some(SpaceId::Ram)); + + // A space with no declared alias says nothing, rather than + // asserting independence. + let plain = AddressSpace::ram(8); + assert_eq!(plain.aliases, None); + + // The field survives a round-trip, so an alias declared by a lifter + // reaches a consumer. + let json = serde_json::to_string(&window).unwrap(); + let back: AddressSpace = serde_json::from_str(&json).unwrap(); + assert_eq!(back.aliases, Some(SpaceId::Ram)); + assert_eq!(back.id, SpaceId::Custom(0x1234)); + } + #[test] fn address_space_optional_endianness_serde() { let space = AddressSpace::new(SpaceId::Ram, "ram", 8); diff --git a/crates/r2il/src/varnode.rs b/crates/r2il/src/varnode.rs index f1b62304d..de7dd402f 100644 --- a/crates/r2il/src/varnode.rs +++ b/crates/r2il/src/varnode.rs @@ -108,6 +108,23 @@ impl Varnode { /// /// Prefer this over touching `.meta` directly — it hides the box, so a /// later change of storage strategy does not ripple through readers. + /// Whether this varnode may be written to a persisted row. + /// + /// False exactly when its space is [`SpaceId::Unresolved`] — a handle the + /// lifter could not resolve, whose only available value was a run-local + /// host pointer. A row containing one cannot be reproduced by a later + /// run, so a write path must refuse it rather than store a fact about a + /// dead process. + /// + /// This is a guard to be CALLED, not an automatic one. `Serialize` still + /// succeeds (it renders the self-describing token `"Unresolved"`), + /// because serde also backs in-process cache keys and diagnostic JSON, + /// where refusing would break correct callers. See [`SpaceId::Unresolved`] + /// for why that trade was made deliberately. + pub fn is_persistable(&self) -> bool { + !self.space.is_unresolved() + } + pub fn meta(&self) -> Option<&VarnodeMetadata> { self.meta.as_deref() } diff --git a/crates/r2sleigh-lift/Cargo.toml b/crates/r2sleigh-lift/Cargo.toml index ffe9563d4..56a75c361 100644 --- a/crates/r2sleigh-lift/Cargo.toml +++ b/crates/r2sleigh-lift/Cargo.toml @@ -17,3 +17,6 @@ x86 = ["sleigh-config/x86"] [[example]] name = "win32_census" required-features = ["x86"] + +[dev-dependencies] +serde_json = "1" diff --git a/crates/r2sleigh-lift/src/context.rs b/crates/r2sleigh-lift/src/context.rs index de8382c9d..0208d1e58 100644 --- a/crates/r2sleigh-lift/src/context.rs +++ b/crates/r2sleigh-lift/src/context.rs @@ -138,6 +138,7 @@ impl LiftContext { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + aliases: None, }; self.arch.add_space(space); return space_id; @@ -159,6 +160,7 @@ impl LiftContext { valid_ranges: Vec::new(), bank_id: None, segment_id: None, + aliases: None, }; self.arch.add_space(space); self.space_map.insert(name.into(), space_id); diff --git a/crates/r2sleigh-lift/src/disasm.rs b/crates/r2sleigh-lift/src/disasm.rs index 9fe145fae..fa28a64fd 100644 --- a/crates/r2sleigh-lift/src/disasm.rs +++ b/crates/r2sleigh-lift/src/disasm.rs @@ -94,7 +94,14 @@ impl<'a> PcodeSource for DisasmInstructionWrapper<'a> { if let Some(space) = spaces.get(idx as usize) { self.disasm.translate_space(space) } else { - SpaceId::Custom(idx as u32) + // `idx` is LOAD/STORE input-0, which Ghidra p-code encodes as the + // host `AddrSpace*` POINTER rather than an index -- so the lookup + // above misses and the value in hand is run-local. + // + // It used to be truncated into `Custom(idx as u32)`. See + // `SpaceId::Unresolved` for why that was wrong in two ways at + // once (unreproducible lift, unreproducible persisted row). + SpaceId::Unresolved } } } @@ -248,6 +255,7 @@ impl Disassembler { SpaceId::Unique => format!("tmp:0x{:x}", vn.offset), SpaceId::Ram => format!("[0x{:x}]:{}", vn.offset, vn.size), SpaceId::Custom(n) => format!("space{}:0x{:x}", n, vn.offset), + SpaceId::Unresolved => format!("unresolved:0x{:x}", vn.offset), } } diff --git a/crates/r2sleigh-lift/src/pcode.rs b/crates/r2sleigh-lift/src/pcode.rs index 1db638c37..4072e52a8 100644 --- a/crates/r2sleigh-lift/src/pcode.rs +++ b/crates/r2sleigh-lift/src/pcode.rs @@ -265,11 +265,13 @@ impl<'a> PcodeSource for RawPcodeSourceWrapper<'a> { } fn space_from_index(&self, idx: u64) -> SpaceId { + // See `SpaceId::Unresolved`: a miss here means `idx` was a host + // pointer, not an index. Dropping it keeps the lift reproducible. self.translator .space_map .get(idx as usize) .copied() - .unwrap_or(SpaceId::Custom(idx as u32)) + .unwrap_or(SpaceId::Unresolved) } } @@ -303,7 +305,7 @@ impl PcodeTranslator { .space_map .get(raw.space_idx as usize) .copied() - .unwrap_or(SpaceId::Custom(raw.space_idx)); + .unwrap_or(SpaceId::Unresolved); Ok(Varnode::new(space, raw.offset, raw.size)) } @@ -690,6 +692,76 @@ fn translate_err(e: translate::TranslateError) -> PcodeError { #[cfg(test)] mod tests { + // ---- the Custom(n) non-determinism falsifier ---------------------- + // + // Runs the REAL fallback in `PcodeTranslator::convert_varnode`, not a + // reconstruction of it. The three space_idx values are the ones actually + // measured across three processes lifting the same x86-64 binary: they + // are truncated host `AddrSpace*` pointers, which is what Ghidra p-code + // puts in a LOAD/STORE's input-0 constant. + // + // DISABLE: restore `.unwrap_or(SpaceId::Custom(raw.space_idx))` in + // `convert_varnode` and this fails on the first assert_eq. + #[test] + fn unresolvable_space_pointers_from_three_runs_convert_identically() { + use super::{PcodeTranslator, RawVarnode}; + + let tr = PcodeTranslator::default_spaces(); + + // Anti-vacuity: these must genuinely miss the space map, or the + // test proves nothing about the fallback. + let measured = [1_062_180_976u32, 2_279_590_000, 1_968_052_336]; + for idx in measured { + assert!( + idx as usize >= 4, + "fixture {idx} must fall outside the 4-entry default map" + ); + } + assert_ne!(measured[0], measured[1]); + assert_ne!(measured[1], measured[2]); + + let lifted: Vec<_> = measured + .iter() + .map(|&idx| { + tr.convert_varnode(&RawVarnode::new(idx, 0x4020, 8)) + .expect("convert") + }) + .collect(); + + // Three different run-local pointers, one identical varnode. + assert_eq!(lifted[0], lifted[1]); + assert_eq!(lifted[1], lifted[2]); + assert!(lifted[0].space.is_unresolved()); + + // And the lift is flagged unpersistable, so a write path can refuse + // an unreproducible row instead of storing a dead process's pointer. + assert!(!lifted[0].is_persistable()); + } + + /// The silence twin: a space index that DOES resolve is untouched. + /// Without this, a `convert_varnode` that returned `Unresolved` for + /// everything would pass the test above. + #[test] + fn a_resolvable_space_index_still_resolves() { + use super::{PcodeTranslator, RawVarnode}; + use r2il::SpaceId; + + let tr = PcodeTranslator::default_spaces(); + for (idx, want) in [ + (0u32, SpaceId::Const), + (1, SpaceId::Ram), + (2, SpaceId::Register), + (3, SpaceId::Unique), + ] { + let vn = tr + .convert_varnode(&RawVarnode::new(idx, 0x10, 4)) + .expect("convert"); + assert_eq!(vn.space, want); + assert!(!vn.space.is_unresolved()); + assert!(vn.is_persistable()); + } + } + use super::*; #[test] diff --git a/crates/r2sleigh-lift/src/translate.rs b/crates/r2sleigh-lift/src/translate.rs index 1b57b4820..408291013 100644 --- a/crates/r2sleigh-lift/src/translate.rs +++ b/crates/r2sleigh-lift/src/translate.rs @@ -326,7 +326,7 @@ mod tests { 0 => SpaceId::Ram, 1 => SpaceId::Register, 2 => SpaceId::Unique, - n => SpaceId::Custom(n as u32), + _ => SpaceId::Unresolved, } } } diff --git a/crates/r2ssa/src/block.rs b/crates/r2ssa/src/block.rs index fff41113a..47505a4bd 100644 --- a/crates/r2ssa/src/block.rs +++ b/crates/r2ssa/src/block.rs @@ -124,6 +124,7 @@ fn varnode_to_name(vn: &Varnode, disasm: &Disassembler) -> String { SpaceId::Const => format!("const:{:x}", vn.offset), SpaceId::Ram => format!("ram:{:x}", vn.offset), SpaceId::Custom(id) => format!("space{}:{:x}", id, vn.offset), + SpaceId::Unresolved => format!("unresolved:{:x}", vn.offset), } } @@ -149,6 +150,7 @@ fn space_name(space: &SpaceId) -> String { SpaceId::Const => "const".to_string(), SpaceId::Unique => "unique".to_string(), SpaceId::Custom(id) => format!("space_{}", id), + SpaceId::Unresolved => "unresolved".to_string(), } } diff --git a/crates/r2ssa/src/naming.rs b/crates/r2ssa/src/naming.rs index c231ba5fe..4e5829ddc 100644 --- a/crates/r2ssa/src/naming.rs +++ b/crates/r2ssa/src/naming.rs @@ -131,6 +131,7 @@ pub fn varnode_to_name(vn: &Varnode, reg_names: Option<&RegisterNameMap>) -> Str SpaceId::Const => format!("const:{:x}", vn.offset), SpaceId::Ram => format!("ram:{:x}", vn.offset), SpaceId::Custom(id) => format!("space{}:{:x}", id, vn.offset), + SpaceId::Unresolved => format!("unresolved:{:x}", vn.offset), } } diff --git a/probes/win32-census/EXECUTION-NOTES.md b/probes/win32-census/EXECUTION-NOTES.md index d8d8e2c2c..e54f76826 100644 --- a/probes/win32-census/EXECUTION-NOTES.md +++ b/probes/win32-census/EXECUTION-NOTES.md @@ -252,11 +252,80 @@ in cache residency — that trade is exactly the kind that can invert. |---|---| | zero-copy state borrow | **done** — `SlabState` borrows, test-pinned | | RAM writable | **done** — enforced by `&mut [u8]`, not a runtime check | -| `Custom(n)` stability | **BROKEN on x86-64** — non-deterministic per process | -| `Ram`/`Custom` aliasing | **open** — same memory, two space ids | -| persisted-row safety | **open** — depends on the `Custom(n)` fix | +| `Custom(n)` stability | **fixed** — the fallback no longer bottles a host pointer; `Custom(u32)` is now name-derived by invariant, unresolvable handles become `SpaceId::Unresolved` | +| `Ram`/`Custom` aliasing | **expressible** — `AddressSpace::aliases` declares "same memory, second identity" without merging the ids (reserve, don't claim). No lifter populates it yet | +| persisted-row safety | **guarded** — an unresolvable space renders as the visible token `"Unresolved"` (never a plausible number) and `Varnode::is_persistable()` is the check a write path calls. Weaker than "cannot be written" — see the correction below | | prefetch: DECODE cache | **not needed yet** — exists as `pre_lift`; triggers named above | | prefetch: LANE hydration (IMAGE) | **ruled** — eager at load for a working set that fits; ahead-of-cursor walk is scale-out | | eager materialization of lifted IR | **no — 749x expansion, 2.2 GB from a 7 MB binary**; lifting must be a cache-miss path | | ontology scale (300 MB–3 GB) | **open** — needs lance-graph-ontology cache in front of lifting; inverts lower->address into address->lift-on-miss | -| `R2ILOp` = 464 bytes | **open prerequisite** — metadata-dominated; ~3.6x reduction is arithmetic, not measured | +| `R2ILOp` = 464 bytes | **done (Move A)** — 464 -> 144 B by boxing `VarnodeMetadata`, 3.22x measured, size pinned by a `const _` assert. (This row read "open prerequisite" for three commits after it landed — a stale row is a doc that lies about its own repo.) | + +### Correction — the `Custom(n)` root cause was misdiagnosed (2026-08-27) + +This file previously described the defect as "`Custom(n)` is non-deterministic", +which named the symptom and left the mechanism open. Reading the lift path +settled it, and the answer changes what the fix had to be. + +There are **two** producers of `SpaceId::Custom`, and only one was broken: + +| producer | input | verdict | +|---|---|---| +| `Disassembler::translate_space` | the space's **name** | **fine** — a byte-sum hash, stable across processes. Never suspected correctly before; it was cleared by reading it, not by assuming. | +| `space_from_index` (3 impls) | LOAD/STORE **input-0** | **the defect** | + +Ghidra p-code encodes a LOAD/STORE's target space as its input-0 constant, +and that constant holds the host **`AddrSpace*` pointer** — not a space index. +So `spaces.get(ptr as usize)` always missed, and the fallback truncated the +pointer into `Custom(ptr as u32)`. That is why the same instruction lifted to +`Custom(1062180976)`, `Custom(2279590000)` and `Custom(1968052336)` on three +runs: ASLR, bottled into a `Serialize`-derived field. + +Two consequences, which is why both open rows closed together: + +1. **The lift was unreproducible.** Same binary, different IR. +2. **The row was unreproducible.** A persisted row keyed on a dead process's + pointer can never be matched again. + +The fix drops the handle instead of bottling it (`SpaceId::Unresolved`), and +makes the drop loud: unresolvable spaces refuse to serialize and fail closed +in `r2conc` rather than guessing RAM. + +**What is NOT fixed.** Resolving the pointer *properly* — mapping it back +through libsla to a named space — is the follow-up. Until then the lift is +honest about not knowing rather than confidently wrong, which is a strictly +better failure but is not the capability. + +### The refusal was tried in `Serialize` first, and that was wrong + +The first cut made `SpaceId`'s `Serialize` **fail** on `Unresolved`, on the +reasoning that a row which cannot be reproduced must not be writable at all. + +It broke **24 of 124** `r2sleigh-plugin` tests, on ordinary x86 fixtures. +Baselined against a stash first, so this was measured as caused rather than +assumed pre-existing. + +The cause is instructive on its own. `types.rs`'s +`function_analysis_cache_key_parts` calls `hash_json_value(&blocks)` — serde +is how an **in-process cache key** is computed, not only how a row is stored. +The same is true of the plugin's diagnostic JSON export. A veto on the type +hits every one of those. + +So the guarantee moved to where persistence actually happens: + +* `Unresolved` **serializes**, as the self-describing token `"Unresolved"` — + a reader can detect it; it can never be mistaken for a space id the way a + truncated pointer could. +* `Varnode::is_persistable()` is the guard a write path calls. +* `r2conc` still **fails closed** at execution (`ConcError::UnresolvedSpace`), + because guessing a slab there would corrupt memory silently. + +State it precisely: **"cannot be written unnoticed", not "cannot be written".** +`r2il` has no row-persist entry point yet (`serialize.rs` persists `ArchSpec`, +not lifted ops), so the guard is available for the write path when it lands +rather than wired into one today. That is the honest boundary. + +**A measurement fell out of the failure.** Those 24 tests were passing before +because the pointer serialized fine — which means they had been hashing ASLR'd +pointers into cache keys all along. Harmless within one process, and proof that +the unresolved path is hit constantly on ordinary x86 code, not in some corner. diff --git a/r2plugin/src/lib.rs b/r2plugin/src/lib.rs index e8a2d75a7..52f477017 100644 --- a/r2plugin/src/lib.rs +++ b/r2plugin/src/lib.rs @@ -1696,6 +1696,9 @@ pub extern "C" fn r2il_block_varnodes( r2il::SpaceId::Ram => 2, r2il::SpaceId::Unique => 3, r2il::SpaceId::Custom(n) => 4 + (n as u8), + // Dedup key only. Every unresolved handle shares one bucket, + // matching `SpaceId`'s own over-match rule. + r2il::SpaceId::Unresolved => u8::MAX, }; let key = (space_id, vn.offset, vn.size); if seen.contains(&key) { @@ -1717,6 +1720,10 @@ pub extern "C" fn r2il_block_varnodes( format!("space{}:0x{:x}", n, vn.offset), space_label(vn.space), ), + r2il::SpaceId::Unresolved => ( + format!("unresolved:0x{:x}", vn.offset), + space_label(vn.space), + ), }; varnodes.push(VarnodeInfo { @@ -1742,6 +1749,7 @@ fn space_label(space: r2il::SpaceId) -> String { r2il::SpaceId::Ram => "ram".to_string(), r2il::SpaceId::Unique => "unique".to_string(), r2il::SpaceId::Custom(id) => format!("custom:{}", id), + r2il::SpaceId::Unresolved => "unresolved".to_string(), } }