r2il: resolve the Custom(n) non-determinism at its source, and guard the row

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"),
         }
     }

crates/r2il/src/serialize.rs

@@ -225,6 +225,8 @@ impl From<AddressSpaceV1> 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<AddressSpaceV2> for AddressSpace {
             valid_ranges: Vec::new(),
             bank_id: None,
             segment_id: None,
+            // Pre-alias rows carry no alias information.
+            aliases: None,
         }
     }
 }

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<String>,
+    /// 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<SpaceId>,
 }
 
 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(<pointer>)` 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);

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()
     }

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"

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);

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),
         }
     }