From f8fd56f5753d7687e3f2de575a2d1f9131217f88 Mon Sep 17 00:00:00 2001 From: Martin Hughes Date: Fri, 3 Jul 2026 17:01:15 +0100 Subject: [PATCH] Ensure that Locals passed to Methods are preserved Since both Locals and Args are stored as references internally, a plain `unwrap_reference` was causing Locals that got passed to methods to be treated as though they had been passed by reference, instead of by value. This corrects that logic, and also takes into account the strange behaviour of the Windows NT interpreter - it treats strings passed via Locals into methods as *always* passed by reference, and never by value. --- src/aml/mod.rs | 31 +++--------- src/aml/object.rs | 113 ++++++++++++++++++++++++++++++++++++++++++ tests/store.asl | 122 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 243 insertions(+), 23 deletions(-) create mode 100644 tests/store.asl diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 2e043463..4687ecbf 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -2381,7 +2381,11 @@ where /// - Locals are overwritten, unless they contain a reference, in which case a store is /// performed to the referenced object with implicit casting /// - Args are overwritten, unless they contain a reference, in which case the referenced - /// object is overwritten + /// object is usually overwritten. References from Arg to Local without an intermediate + /// `RefOf` cause the Arg to be overwritten. + /// (see [issue #313](https://github.com/rust-osdev/acpi/issues/313)) + /// - Args that ultimately refer to a string *always* overwrite the string and not the Arg, + /// as per the Windows NT behaviour (see `tests/store.asl`) /// - Index references behave the same as locals /// - Named objects are stored into, with implicit casting fn do_store(&self, target: WrappedObject, object: WrappedObject) -> Result { @@ -2389,29 +2393,10 @@ where let token = self.object_token.lock(); match unsafe { target.gain_mut(&token) } { - Object::Reference { kind, inner } => { - let (target_object, overwrite) = match kind { - ReferenceKind::Named => (inner.clone().unwrap_reference(), false), - ReferenceKind::Local | ReferenceKind::Index => { - if let Object::Reference { kind: _, inner: ref inner_inner } = **inner { - (inner_inner.clone(), false) - } else { - (inner.clone().unwrap_transparent_reference(), true) - } - } - ReferenceKind::Arg => { - if let Object::Reference { kind: _, inner: ref inner_inner } = **inner { - (inner_inner.clone(), true) - } else { - (inner.clone().unwrap_transparent_reference(), true) - } - } - ReferenceKind::RefOf | ReferenceKind::Unresolved => { - return Err(AmlError::StoreToInvalidReferenceType); - } - }; + Object::Reference { .. } => { + let (target_object, implicit_cast_reqd) = target.unwrap_ref_for_store()?; - if overwrite { + if !implicit_cast_reqd { unsafe { *target_object.gain_mut(&token) = (*object).clone(); } diff --git a/src/aml/object.rs b/src/aml/object.rs index bfc03b41..923f8b86 100644 --- a/src/aml/object.rs +++ b/src/aml/object.rs @@ -154,6 +154,63 @@ impl WrappedObject { } } } + + /// Unwrap a reference that is about to be stored to - find the target object. + /// + /// Take into account the store rules as enumerated by [`Interpreter::do_store`] + /// + /// Returns a tuple containing: + /// - The object that should be modified + /// - A boolean indicating whether an implicit cast should occur before the store + pub fn unwrap_ref_for_store(self) -> Result<(WrappedObject, bool), AmlError> { + let Object::Reference { .. } = *self else { + return Err(AmlError::ObjectNotOfExpectedType { expected: ObjectType::Reference, got: self.typ() }); + }; + + let mut target = self; + let mut implicit_cast_reqd = false; + + // Unwrap references, but with the following caveats: + // - If an Arg -> Local reference is found, we return the Arg so that it can be stored in. + // Except... + // - The Windows NT interpreter allows strings stored in locals that are then passed as args + // to be modified even if they aren't passed by reference... so we must continue + // unwrapping to see if the end of the reference chain is a String or not. If it is, + // return that instead (a bit like a normal `unwrap_reference`) + // + // See issue 313 and the `store.asl` tests for more details. + let mut found_arg_to_local: Option> = None; + + loop { + let Object::Reference { kind, ref inner } = *target else { + if target.typ() == ObjectType::String { + return Ok((target.clone(), true)); + } + return found_arg_to_local.unwrap_or_else(|| Ok((target.clone(), implicit_cast_reqd))); + }; + + implicit_cast_reqd = match kind { + ReferenceKind::Named => true, + ReferenceKind::Local | ReferenceKind::Index | ReferenceKind::RefOf => false, + ReferenceKind::Arg => { + if found_arg_to_local.is_none() + && matches!(**inner, Object::Reference { kind: ReferenceKind::Local, inner: _ }) + { + found_arg_to_local = Some(Ok((inner.clone(), implicit_cast_reqd))); + } + false + } + ReferenceKind::Unresolved => { + if found_arg_to_local.is_none() { + found_arg_to_local = Some(Err(AmlError::StoreToInvalidReferenceType)); + } + implicit_cast_reqd + } + }; + + target = inner.clone(); + } + } } impl ops::Deref for WrappedObject { @@ -653,4 +710,60 @@ mod tests { assert_eq!(buffer_field.to_integer(IntegerSize::EightBytes).unwrap(), 0x0000000f_00000000); } + + #[test] + fn store_local_ref_to_local() { + // As may be encountered in the last line of: + // Local1 = RefOf(Local0) + // Local1 = 2 (the actual store is omitted) + let local0 = Object::Reference { kind: ReferenceKind::Local, inner: Object::Integer(1).wrap() }.wrap(); + let ref_of = Object::Reference { kind: ReferenceKind::RefOf, inner: local0 }.wrap(); + let local1 = Object::Reference { kind: ReferenceKind::Local, inner: ref_of }.wrap(); + + let target = local1.unwrap_ref_for_store(); + let target = target.unwrap(); + + let target_obj = &*target.0; + let Object::Integer(x) = target_obj else { + panic!("Incorrect type"); + }; + assert_eq!(*x, 1); + } + + #[test] + fn store_arg_ref_to_local() { + // As if a Local was passed as an argument to a method, and then Arg0 were stored to e.g.: + // Local0 = 1 + // MEFD(Local0) + // ... and inside MEFD: Arg0 = 2 (the actual store is omitted) + let local0 = Object::Reference { kind: ReferenceKind::Local, inner: Object::Integer(1).wrap() }.wrap(); + let arg0 = Object::Reference { kind: ReferenceKind::Arg, inner: local0.clone() }.wrap(); + + let target = arg0.unwrap_ref_for_store(); + let (target, implicit_cast_reqd) = target.unwrap(); + + assert!(Arc::ptr_eq(&target.0, &local0.0)); + assert!(!implicit_cast_reqd); + } + + #[test] + fn store_arg_ref_of_local() { + // As may be encountered in the last line of: + // Local0 = 1 + // Arg0 = RefOf(Local0) + // Arg0 = 2 (the actual store is omitted) + let local0 = Object::Reference { kind: ReferenceKind::Local, inner: Object::Integer(1).wrap() }.wrap(); + let ref_of = Object::Reference { kind: ReferenceKind::RefOf, inner: local0 }.wrap(); + let arg0 = Object::Reference { kind: ReferenceKind::Arg, inner: ref_of }.wrap(); + + let target = arg0.unwrap_ref_for_store(); + let (target, implicit_cast_reqd) = target.unwrap(); + + let target_obj = &*target; + let Object::Integer(x) = target_obj else { + panic!("Incorrect type"); + }; + assert_eq!(*x, 1); + assert!(!implicit_cast_reqd); + } } diff --git a/tests/store.asl b/tests/store.asl new file mode 100644 index 00000000..f0bf0bc8 --- /dev/null +++ b/tests/store.asl @@ -0,0 +1,122 @@ +// Check that store handles simple references correctly +// +// Tests T1 - T4 are very basic, to ensure any trivial errors in `do_store` are caught. +// +// These tests don't check any conversions - it's assumed that references and conversions are orthogonal. +DefinitionBlock ("", "DSDT", 1, "RSACPI", "TESTTABL", 0xF0F0F0F0) +{ + Name(FCNT, 0) + + Method (CHEK, 2) { + If (Arg0 != Arg1) { + FCNT++ + } + } + + Method (T1) { + Name(V1, 1) + V1 = 2 + CHEK(V1, 2) + } + + Method (T2) { + Name(V1, 1) + Alias(V1, V2) + V2 = 2 + CHEK(V1, 2) + } + + Method (T3) { + Local1 = 1 + Local2 = Local1 + Local2 = 2 + CHEK(Local1, 1) + } + + Method (T4) { + Local1 = 1 + Local2 = RefOf(Local1) + Local2 = 2 + CHEK(Local1, 2) + } + + Method (INR5, 1) { + Arg0 = 5 + } + + Method (T5) { + Local1 = 1 + INR5(Local1) + CHEK (Local1, 1) + } + + Method (T6) { + Local1 = 1 + INR5(RefOf(Local1)) + CHEK (Local1, 5) + } + + Method (T7, 1) { + Local1 = 1 + Arg0 = RefOf(Local1) + Arg0 = 2 + CHEK (Local1, 2) + } + + // Test 8 is adapted from uACPI's `references-3.asl`. To quote that test: + // "This test seems bogus but it's actually correct, it produces the same output on NT." + Method (INR8, 1, NotSerialized) + { + Local0 = RefOf(Arg0) + + // WHY? in little-endian ASCII + Local0 = 0x3F594857 + } + + Method (T8) + { + Local0 = "MyST" + INR8(Local0) + CHEK(Local0, "WHY?") + } + + // This is the same as `T8` but with an extra function call to see if the Windows behaviour is + // limited to one level of the stack - but it is not, multiple calls behave the same as a + // single call. + Method (T8A) { + Local0 = "MyST" + IN8A(Local0) + CHEK(Local0, "WHY?") + } + + Method (IN8A, 1, NotSerialized) { + INR8(Arg0) + } + + // Test 8 not withstanding, non-string "pass by value" argument types show the expected behavior. + Method (INR9, 1) { + Local0 = RefOf(Arg0) + Local0 = 9 + } + + Method (T9) { + Local0 = 1 + INR9(Local0) + CHEK(Local0, 1) + } + + Method (MAIN, 0, NotSerialized) { + T1() + T2() + T3() + T4() + T5() + T6() + T7(0) + T8() + T8A() + T9() + + Return (FCNT) + } +}