Directly test a stream of opcodes

src/aml/mod.rs

@@ -20,6 +20,7 @@ pub mod namespace;
 pub mod object;
 pub mod op_region;
 pub mod pci_routing;
+pub mod pkglength;
 pub mod resource;
 
 use crate::{
@@ -28,6 +29,7 @@ use crate::{
     Handle,
     Handler,
     PhysicalMapping,
+    aml::pkglength::decode_stream as decode_pkglength,
     platform::AcpiPlatform,
     registers::{FixedRegisters, Pm1ControlBit},
     sdt::{SdtHeader, facs::Facs, fadt::Fadt},
@@ -3200,19 +3202,7 @@ impl MethodContext {
     }
 
     fn pkglength(&mut self) -> Result<usize, AmlError> {
-        let lead_byte = self.next()?;
-        let byte_count = lead_byte.get_bits(6..8);
-        assert!(byte_count < 4);
-
-        if byte_count == 0 {
-            Ok(lead_byte.get_bits(0..6) as usize)
-        } else {
-            let mut length = lead_byte.get_bits(0..4) as usize;
-            for i in 0..byte_count {
-                length |= (self.next()? as usize) << (4 + i * 8);
-            }
-            Ok(length)
-        }
+        decode_pkglength(|| self.next())
     }
 
     fn namestring(&mut self) -> Result<AmlName, AmlError> {

src/aml/pkglength.rs

@@ -0,0 +1,148 @@
+//! pkglength encoding and decoding.
+
+use alloc::{vec, vec::Vec};
+use bit_field::BitField;
+use core::fmt::{Display, Formatter};
+
+/// Indicates an attempt to encode a pkglength that is too long (>= 2 ^ 28).
+#[derive(Clone, Debug)]
+pub struct PkglengthTooLongError {}
+
+impl Display for PkglengthTooLongError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result {
+        write!(f, "pkglength too long")
+    }
+}
+
+impl core::error::Error for PkglengthTooLongError {}
+
+/// Encode a pkglength field.
+///
+/// This is a variable length field used to define the length of other variable-length items in
+/// ACPI - see [the spec](https://uefi.org/specs/ACPI/6.6/20_AML_Specification.html#package-length-encoding)
+/// for details.
+///
+/// This is less straightforward than it could be, because pkglength needs to include the length of
+/// the pkglength field that gets output. This function adds that extra length.
+///
+/// Returns an error if length >= 2^28. Otherwise, returns the encoded pkglength in a vec, LSB-first.
+pub fn encode(data_length: u32) -> Result<Vec<u8>, PkglengthTooLongError> {
+    let extra_length = match data_length {
+        0..63 => 1,
+        63..0xFFE => 2,
+        0xFFE..0xFFFFD => 3,
+        _ => 4,
+    };
+    let result = encode_raw(data_length + extra_length);
+    result.inspect(|result| {
+        assert_eq!(result.len(), extra_length as usize);
+    })
+}
+
+/// Encode a pkglength field, without taking into account the extra length of the pkglength field.
+///
+/// Most callers should use [`encode`] instead.
+///
+/// Returns an error if length >= 2^28. Otherwise, returns the encoded pkglength in a vec, LSB-first.
+pub fn encode_raw(mut length: u32) -> Result<Vec<u8>, PkglengthTooLongError> {
+    if length & 0xF0000000 != 0 {
+        // Must be less than 2 ^ 28
+        return Err(PkglengthTooLongError {});
+    }
+
+    if length < 64 {
+        Ok(vec![length as u8])
+    } else {
+        let mut result = vec![(length & 0xF) as u8];
+        length >>= 4;
+
+        while length != 0 {
+            result.push((length & 0xff) as u8);
+            length >>= 8;
+        }
+
+        let num_bytes = result.len() as u8 - 1;
+        result[0] |= num_bytes << 6;
+
+        Ok(result)
+    }
+}
+
+/// Decode a pkglength field from a stream
+///
+/// If the stream returns an error, that error is returned. Otherwise, the decoded length is
+/// returned.
+///
+/// `stream_next` must return the next byte in the stream when called (or an error, which is passed
+/// through)
+pub fn decode_stream<T>(mut stream_next: impl FnMut() -> Result<u8, T>) -> Result<usize, T> {
+    let lead_byte = stream_next()?;
+    let byte_count = lead_byte.get_bits(6..8);
+    assert!(byte_count < 4);
+
+    if byte_count == 0 {
+        Ok(lead_byte.get_bits(0..6) as usize)
+    } else {
+        let mut length = lead_byte.get_bits(0..4) as usize;
+        for i in 0..byte_count {
+            length |= (stream_next()? as usize) << (4 + i * 8);
+        }
+        Ok(length)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn basic_round_trip() {
+        let length: u32 = 0x12345;
+        let encoded = encode_raw(length).unwrap();
+        let mut i = encoded.iter();
+        let decoded = decode_stream(|| i.next().copied().ok_or(())).unwrap();
+        assert_eq!(decoded, length as usize);
+    }
+
+    #[test]
+    fn less_than_64() {
+        let length: u32 = 0x12;
+        let encoded = encode_raw(length).unwrap();
+        assert_eq!(encoded, vec![0x12]);
+
+        let mut i = encoded.iter();
+        let decoded = decode_stream(|| i.next().copied().ok_or(())).unwrap();
+        assert_eq!(decoded, 0x12);
+    }
+
+    #[test]
+    fn encodes_zero() {
+        let encoded = encode_raw(0).unwrap();
+        assert_eq!(encoded, vec![0]);
+    }
+
+    fn round_trip_length(length: u32) -> usize {
+        let encoded = encode(length).unwrap();
+        let mut i = encoded.iter();
+        decode_stream(|| i.next().copied().ok_or(())).unwrap()
+    }
+
+    #[test]
+    fn extra_length_correct() {
+        assert_eq!(round_trip_length(62), 63); // An increase of a single byte
+        // 63 bytes of payload requires two bytes of pkglength
+        assert_eq!(round_trip_length(63), 65);
+
+        // A random test:
+        assert_eq!(round_trip_length(0x12345), 0x12348);
+
+        // Simply check that there's no errors around the boundaries - this tells us the maths to
+        // calculate `extra_length` is correct.
+        for i in 0xFF9..0x1004 {
+            encode(i).unwrap();
+        }
+        for i in 0xFFFF9..0x100004 {
+            encode(i).unwrap();
+        }
+    }
+}

tests/stray_opcodes_test.rs

@@ -0,0 +1,14 @@
+mod test_infra;
+
+use crate::test_infra::run_opcodes_test;
+use aml_test_tools::handlers::null_handler::NullHandler;
+
+#[test]
+fn test_stray_opcode() {
+    // Make sure that a stray `One` opcode is ignored (see issue #289)
+    // The first opcode is `One`, the remainder are `Return (0)`.
+    let opcodes: Vec<u8> = vec![0x01, 0xA4, 0x0A, 0x00];
+
+    let handler = NullHandler;
+    run_opcodes_test(&opcodes, handler);
+}

tests/test_infra/mod.rs

@@ -1,12 +1,14 @@
 use acpi::Handler;
-use aml_test_tools::{
-    RunTestResult,
-    TestResult,
-    handlers::logging_handler::LoggingHandler,
-    new_interpreter,
-    run_test_for_string,
-};
+use aml_test_tools::{RunTestResult, TestResult, handlers::logging_handler::LoggingHandler, new_interpreter, run_test_for_string, run_test_for_opcodes};
 
+// The following two functions are very similar in structure, but whilst there are only two of them
+// it's not worth adding complexity to make them DRY.
+
+/// Run a test against an ASL string.
+///
+/// The string `asl` represents a compile-able ASL string, so needs to include the `DefinitionBlock`
+/// statement.
+#[allow(dead_code)]
 pub fn run_aml_test(asl: &'static str, handler: impl Handler) {
     // Tests calling `run_aml_test` don't do much else, and we usually want logging, so initialize it here.
     let _ = pretty_env_logger::try_init();
@@ -17,3 +19,19 @@ pub fn run_aml_test(asl: &'static str, handler: impl Handler) {
     let result = run_test_for_string(asl, interpreter, &None);
     assert!(matches!(result, RunTestResult::Pass(_)), "Test failed with: {:?}", TestResult::from(&result));
 }
+
+/// Run a test against a sequence of AML opcodes.
+///
+/// The provided opcodes are wrapped in a `MAIN` method and also have a table header prepended. The
+/// `MAIN` function is executed as part of the test, so `opcodes` only needs to include the actual
+/// opcodes to execute.
+#[allow(dead_code)]
+pub fn run_opcodes_test(opcodes: &[u8], handler: impl Handler) {
+    let _ = pretty_env_logger::try_init();
+
+    let logged_handler = LoggingHandler::new(handler);
+    let interpreter = new_interpreter(logged_handler);
+
+    let result = run_test_for_opcodes(opcodes, interpreter, &None);
+    assert!(matches!(result, RunTestResult::Pass(_)), "Test failed with: {:?}", TestResult::from(&result));
+}

tools/aml_test_tools/src/lib.rs

@@ -16,8 +16,8 @@ use acpi::{
     Handler,
     PhysicalMapping,
     address::MappedGas,
-    aml::{AmlError, Interpreter, namespace::AmlName},
-    sdt::{Signature, facs::Facs},
+    aml::{AmlError, Interpreter, namespace::AmlName, pkglength},
+    sdt::{SdtHeader, Signature, facs::Facs},
 };
 use log::{error, trace};
 use std::{
@@ -330,6 +330,61 @@ where
     run_test(tables, interpreter, expected_result)
 }
 
+/// Test a sequence of opcodes.
+///
+/// This may be useful for testing a sequence of opcodes that are seen in the wild, but which
+/// can't be generated by the AML compiler. (For example, see issue #289 where stray `One` opcodes
+/// were seen in AML - the `iasl` compiler will not generate this sequence.)
+///
+/// The provided opcodes are wrapped in a `MAIN` method and also have a table header prepended.
+///
+/// As with all other tests: if `MAIN` returns either 0 or undefined, then the test is considered
+/// successful.
+pub fn run_test_for_opcodes<T>(
+    opcodes: &[u8],
+    interpreter: Interpreter<T>,
+    expected_result: &Option<ExpectedResult>,
+) -> RunTestResult<T>
+where
+    T: Handler,
+{
+    // DefMethod := MethodOp PkgLength NameString MethodFlags TermList
+    let mut method_bytes: Vec<u8> = vec![0x14];
+    // PkgLength - add 5 to cover the length of the method name and flags.
+    method_bytes.extend(pkglength::encode(opcodes.len() as u32 + 5).unwrap().iter());
+    method_bytes.extend(r"MAIN".as_bytes()); // NameString
+    method_bytes.extend([0]); // MethodFlags
+    // Nothing for termlist
+
+    let table_header = SdtHeader {
+        signature: Signature::DSDT,
+        length: (size_of::<SdtHeader>() + opcodes.len() + method_bytes.len()) as u32,
+        revision: 1,
+        checksum: 0, // The crate doesn't check checksums, so no need to set it.
+        oem_id: [0; 6],
+        oem_table_id: [0; 8],
+        oem_revision: 1,
+        creator_id: [0; 4],
+        creator_revision: 1,
+    };
+
+    // Safe as we retain ownership of table_header and then drop both it and the slice at the end
+    // of this function.
+    let table_hdr_slice =
+        unsafe { std::slice::from_raw_parts(&table_header as *const _ as *const u8, size_of::<SdtHeader>()) };
+
+    let mut table_data = vec![];
+    table_data.extend(table_hdr_slice);
+    table_data.extend(method_bytes);
+    table_data.extend(opcodes);
+
+    let Ok(tables) = bytes_to_tables(&table_data) else {
+        return RunTestResult::Failed(interpreter, TestFailureReason::TablesErr);
+    };
+
+    run_test(tables, interpreter, expected_result)
+}
+
 /// Internal function to create a temporary script file from an ASL string, plus to calculate the
 /// name of the post-compilation AML file.
 fn create_script_file(asl: &'static str) -> TempScriptFile {