diff --git a/src/aml/mod.rs b/src/aml/mod.rs index 44ef3583..e81eb95e 100644 --- a/src/aml/mod.rs +++ b/src/aml/mod.rs @@ -142,6 +142,21 @@ const INTERPRETER_REVISION: u64 = 1; /// to make us loop forever. const MAX_NAME_PATH_INDIRECTIONS: usize = 8; +// The following constants are public as they are referenced from the AmlError docs. +/// How many nested function calls should we allow? This is a fairly arbitrary figure that could be +/// changed with experience if it proves too small. +/// +/// The interpreter doesn't allocate a stack frame on the processor stack for each AML method call, +/// so we're not worried about overflowing the kernel or process stack. This is a basic way to +/// ensure the AML is not stuck in an infinite recursion (which would deadlock the caller) +pub const MAX_STACK_DEPTH: usize = 1000; + +/// The maximum length of time a single loop should be allowed to execute for, in nanoseconds. +/// +/// This is a fairly arbitrary limit to protect against infinite loops and the risk of system +/// deadlock. +pub const LOOP_TIMEOUT_NS: u64 = 10_000_000_000; + impl BaseInterpreter where H: Handler, @@ -969,6 +984,10 @@ where context.retire_op(op); } Opcode::InternalMethodCall => { + if context_stack.len() >= MAX_STACK_DEPTH { + return Err(AmlError::MethodStackExceeded); + } + extract_args!(op[0..2] => [Argument::Object(method), Argument::Namestring(method_scope)]); let args = op.arguments[2..] .iter() @@ -1174,13 +1193,8 @@ where continue; } - BlockKind::While { start_pc } => { - /* - * Go round again, and create a new in-flight op to have a look at the - * predicate. - */ - context.current_block.pc = start_pc; - context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); + BlockKind::While { .. } => { + self.continue_loop(&mut context)?; continue; } } @@ -1781,26 +1795,16 @@ where let pkg_length = context.pkglength()?; let remaining_length = pkg_length - (context.current_block.pc - start_pc); context.start_new_block( - BlockKind::While { start_pc: context.current_block.pc }, + BlockKind::While { + start_pc: context.current_block.pc, + start_nanos: self.handler.nanos_since_boot(), + }, remaining_length, ); context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); } Opcode::Continue => { - if let BlockKind::While { start_pc } = &context.current_block.kind { - context.current_block.pc = *start_pc; - } else { - loop { - let Some(block) = context.block_stack.pop() else { - Err(AmlError::ContinueOutsideOfWhile)? - }; - if let BlockKind::While { start_pc } = block.kind { - context.current_block.pc = start_pc; - break; - } - } - } - context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); + self.continue_loop(&mut context)?; } Opcode::Break => { if let BlockKind::While { .. } = &context.current_block.kind { @@ -2855,6 +2859,33 @@ where }; Ok(PciAddress::new(seg as u16, bus as u8, device as u8, function as u8)) } + + /// Return to the beginning of a While loop - either because `Continue` was executed or + /// because we reached the end of the While block. + fn continue_loop(&self, context: &mut MethodContext) -> Result<(), AmlError> { + let start: u64; + + if let BlockKind::While { start_pc, start_nanos } = &context.current_block.kind { + context.current_block.pc = *start_pc; + start = *start_nanos; + } else { + loop { + let Some(block) = context.block_stack.pop() else { Err(AmlError::ContinueOutsideOfWhile)? }; + if let BlockKind::While { start_pc, start_nanos } = block.kind { + context.current_block.pc = start_pc; + start = start_nanos; + break; + } + } + } + + if self.handler.nanos_since_boot() - start > LOOP_TIMEOUT_NS { + return Err(AmlError::LoopTimeout); + } + + context.start(OpInFlight::new(Opcode::While, &[ResolveBehaviour::TermArg])); + Ok(()) + } } /// A `MethodContext` represents a piece of running AML code - either a real method, or the @@ -2898,6 +2929,7 @@ pub enum BlockKind { IfThenBranch, While { start_pc: usize, + start_nanos: u64, }, } @@ -3027,11 +3059,8 @@ impl MethodContext { if args.len() != flags.arg_count() { return Err(AmlError::MethodArgCountIncorrect); } - let block = Block { - stream: code.clone(), - pc: 0, - kind: BlockKind::Method { method_scope: scope.clone() }, - }; + let block = + Block { stream: code.clone(), pc: 0, kind: BlockKind::Method { method_scope: scope.clone() } }; let args = core::array::from_fn(|i| { if let Some(arg) = args.get(i) { arg.clone() } else { Object::Uninitialized.wrap() } }); @@ -3562,6 +3591,13 @@ pub enum AmlError { /// An internal interpreter error has occured, and the interpreter has been left in an unknown /// state. More information may be given in the contained value. InternalError(String), + + /// The maximum stack depths of method calls [`MAX_STACK_DEPTH`] has been reached, so the method + /// call has been aborted. + MethodStackExceeded, + + /// The maximum length of time a loop ([`LOOP_TIMEOUT_NS`]) can execute for has been exceeded. + LoopTimeout, } #[derive(Debug, Clone, Copy, PartialEq)] diff --git a/tests/infinite_recursion.rs b/tests/infinite_recursion.rs new file mode 100644 index 00000000..8329b51c --- /dev/null +++ b/tests/infinite_recursion.rs @@ -0,0 +1,20 @@ +mod test_infra; +use acpi::aml::AmlError; +use aml_test_tools::{RunTestResult, TestFailureReason, handlers::null_handler::NullHandler}; +use std::assert_matches; + +#[test] +fn infinite_method_recursion() { + const ASL: &str = r#" +DefinitionBlock("", "DSDT", 1, "RSACPI", "UACPI", 1) { + Name(X, 0) + Method(INF) { + X++ + INF() + } + INF() +}"#; + + let r = test_infra::run_aml_test_with_result(ASL, NullHandler); + assert_matches!(r, RunTestResult::Failed(_, TestFailureReason::ParseFail(AmlError::MethodStackExceeded))); +} diff --git a/tests/test_infra/mod.rs b/tests/test_infra/mod.rs index 45f4fc7b..aa94b987 100644 --- a/tests/test_infra/mod.rs +++ b/tests/test_infra/mod.rs @@ -15,23 +15,32 @@ use std::str::FromStr; // `run_aml_test` and `run_opcodes_test` 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. +/// Run a test against an ASL string and check for successful execution. /// /// 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: H) -> Interpreter> { - // Tests calling `run_aml_test` don't do much else, and we usually want logging, so initialize it here. + let result = run_aml_test_with_result(asl, handler); + match result { + RunTestResult::Pass(interpreter) => interpreter, + result => panic!("Test failed with: {:?}", TestResult::from(&result)), + } +} + +/// Run a test against an ASL string and return the raw result. +/// +/// The string `asl` represents a compile-able ASL string, so needs to include the `DefinitionBlock` +/// statement. +#[allow(dead_code)] +pub fn run_aml_test_with_result(asl: &'static str, handler: H) -> RunTestResult> { + // Tests calling the test functions don't do much else, and we usually want logging, so initialize it here. let _ = pretty_env_logger::try_init(); let logged_handler = LoggingHandler::new(handler); let interpreter = new_interpreter(logged_handler); - let result = run_test_for_string(asl, interpreter, &None); - match result { - RunTestResult::Pass(interpreter) => interpreter, - result => panic!("Test failed with: {:?}", TestResult::from(&result)), - } + run_test_for_string(asl, interpreter, &None) } /// Evaluate an object without arguments and return its unwrapped value. diff --git a/tests/while.asl b/tests/while.asl index 7d6c8d85..e6b4a9b6 100644 --- a/tests/while.asl +++ b/tests/while.asl @@ -1,31 +1,62 @@ DefinitionBlock("while.aml", "DSDT", 1, "RSACPI", "WHILE", 1) { - Name(X, 0) - While (X < 5) { - X++ + Name(FCNT, 0) + + Method (CHEK, 2) { + If (Arg0 != Arg1) { + FCNT++ + } } - // Test `DefBreak` - Y should only make it to 5 - Name(Y, 0) - While (Y < 10) { - If (Y >= 5) { - Break + Method(T1) { + Name(X, 0) + While (X < 5) { + X++ } - Y++ + CHEK(X, 5) } - // Test `DefContinue` - Z should remain at zero - Name(CNT, 0) - Name(Z, 0) - While (CNT < 5) { - CNT++ - Continue - Z++ + Method(T2) { + // Test `DefBreak` - Y should only make it to 5 + Name(Y, 0) + While (Y < 10) { + If (Y >= 5) { + Break + } + + Y++ + } + CHEK(Y, 5) } - // Test `Decrement` in the predicate - common pattern - Local0 = 5 - While (Local0--) { - Continue + Method(T3) { + // Test `DefContinue` - Z should remain at zero + Name(CNT, 0) + Name(Z, 0) + While (CNT < 5) { + CNT++ + Continue + Z++ + } + CHEK(Z, 0) + CHEK(CNT, 5) + } + + Method(T4) { + // Test `Decrement` in the predicate - common pattern + Local0 = 5 + While (Local0--) { + Continue + } + CHEK(Local0, 0) + } + + Method(MAIN) { + T1() + T2() + T3() + T4() + + Return(FCNT) } } diff --git a/tests/while_loop.rs b/tests/while_loop.rs new file mode 100644 index 00000000..238574e9 --- /dev/null +++ b/tests/while_loop.rs @@ -0,0 +1,23 @@ +mod test_infra; +use acpi::aml::AmlError; +use aml_test_tools::{ + RunTestResult, + TestFailureReason, + handlers::{null_handler::NullHandler, sys_timer_handler::SystemTimerHandler}, +}; +use std::assert_matches; + +#[test] +fn infinite_while_loop() { + const ASL: &str = r#" +DefinitionBlock("", "DSDT", 1, "RSACPI", "UACPI", 1) { + Name(X, 0) + While (1) { + X++ + } +}"#; + + let handler = SystemTimerHandler::new(NullHandler, 100); + let r = test_infra::run_aml_test_with_result(ASL, handler); + assert_matches!(r, RunTestResult::Failed(_, TestFailureReason::ParseFail(AmlError::LoopTimeout))); +} diff --git a/tools/aml-test-tools/src/handlers/mod.rs b/tools/aml-test-tools/src/handlers/mod.rs index 85d58708..99315c30 100644 --- a/tools/aml-test-tools/src/handlers/mod.rs +++ b/tools/aml-test-tools/src/handlers/mod.rs @@ -7,3 +7,4 @@ pub mod listed_response_handler; pub mod logging_handler; pub mod null_handler; pub mod std_test_handler; +pub mod sys_timer_handler; diff --git a/tools/aml-test-tools/src/handlers/sys_timer_handler.rs b/tools/aml-test-tools/src/handlers/sys_timer_handler.rs new file mode 100644 index 00000000..7dda035a --- /dev/null +++ b/tools/aml-test-tools/src/handlers/sys_timer_handler.rs @@ -0,0 +1,157 @@ +//! A wrapper around another [`Handler`] that checks for the correct sequence of commands in a test. + +use acpi::{Handle, Handler, RawPhysicalMapping, aml::AmlError}; +use pci_types::PciAddress; +use std::{ + thread::sleep, + time::{Duration, Instant}, +}; + +/// A wrapper around another [`Handler`] that enables the handler to use the system's timing +/// capabilities. +/// +/// All commands are forwarded except [`sleep`], [`stall`] and [`nanos_since_boot`]. +#[derive(Clone, Debug)] +pub struct SystemTimerHandler +where + H: Handler + Clone, +{ + next_handler: H, + start_time: Instant, + scale_factor: u64, +} + +impl SystemTimerHandler +where + H: Handler + Clone, +{ + /// Construct a new `SystemTimerHandler` + /// + /// * `next_handler` is the handler to forward non-timing method calls to. + /// * `scale_factor` effectively accelerates time by the given integer multiple. For example, a + /// 10-second wait with a scale factor of 5 would lead to a 2-second wait. Note: Large + /// multiples combined with small waits may lead to reduced accuracy or zero-length waits. + pub fn new(next_handler: H, scale_factor: u64) -> Self { + Self { next_handler, start_time: Instant::now(), scale_factor } + } +} + +impl Handler for SystemTimerHandler +where + H: Handler + Clone, +{ + unsafe fn map_physical_region(&self, physical_address: usize, size: usize) -> RawPhysicalMapping { + unsafe { self.next_handler.map_physical_region::(physical_address, size) } + } + + unsafe fn unmap_physical_region(&self, region: RawPhysicalMapping) { + unsafe { + self.next_handler.unmap_physical_region(region); + } + } + + fn read_u8(&self, address: usize) -> u8 { + self.next_handler.read_u8(address) + } + + fn read_u16(&self, address: usize) -> u16 { + self.next_handler.read_u16(address) + } + + fn read_u32(&self, address: usize) -> u32 { + self.next_handler.read_u32(address) + } + + fn read_u64(&self, address: usize) -> u64 { + self.next_handler.read_u64(address) + } + + fn write_u8(&self, address: usize, value: u8) { + self.next_handler.write_u8(address, value); + } + + fn write_u16(&self, address: usize, value: u16) { + self.next_handler.write_u16(address, value); + } + + fn write_u32(&self, address: usize, value: u32) { + self.next_handler.write_u32(address, value); + } + + fn write_u64(&self, address: usize, value: u64) { + self.next_handler.write_u64(address, value); + } + + fn read_io_u8(&self, port: u16) -> u8 { + self.next_handler.read_io_u8(port) + } + + fn read_io_u16(&self, port: u16) -> u16 { + self.next_handler.read_io_u16(port) + } + + fn read_io_u32(&self, port: u16) -> u32 { + self.next_handler.read_io_u32(port) + } + + fn write_io_u8(&self, port: u16, value: u8) { + self.next_handler.write_io_u8(port, value); + } + + fn write_io_u16(&self, port: u16, value: u16) { + self.next_handler.write_io_u16(port, value); + } + + fn write_io_u32(&self, port: u16, value: u32) { + self.next_handler.write_io_u32(port, value); + } + + fn read_pci_u8(&self, address: PciAddress, offset: u16) -> u8 { + self.next_handler.read_pci_u8(address, offset) + } + + fn read_pci_u16(&self, address: PciAddress, offset: u16) -> u16 { + self.next_handler.read_pci_u16(address, offset) + } + + fn read_pci_u32(&self, address: PciAddress, offset: u16) -> u32 { + self.next_handler.read_pci_u32(address, offset) + } + + fn write_pci_u8(&self, address: PciAddress, offset: u16, value: u8) { + self.next_handler.write_pci_u8(address, offset, value); + } + + fn write_pci_u16(&self, address: PciAddress, offset: u16, value: u16) { + self.next_handler.write_pci_u16(address, offset, value); + } + + fn write_pci_u32(&self, address: PciAddress, offset: u16, value: u32) { + self.next_handler.write_pci_u32(address, offset, value); + } + + fn nanos_since_boot(&self) -> u64 { + ((Instant::now() - self.start_time).as_nanos() as u64) * self.scale_factor + } + + fn stall(&self, microseconds: u64) { + // There's no `std` equivalent to stall, and sleep is probably OK for a test environment. + sleep(Duration::from_micros(microseconds / self.scale_factor)); + } + + fn sleep(&self, milliseconds: u64) { + sleep(Duration::from_millis(milliseconds / self.scale_factor)); + } + + fn create_mutex(&self) -> Handle { + self.next_handler.create_mutex() + } + + fn acquire(&self, mutex: Handle, timeout: u16) -> Result<(), AmlError> { + self.next_handler.acquire(mutex, timeout) + } + + fn release(&self, mutex: Handle) { + self.next_handler.release(mutex); + } +} diff --git a/tools/aml-test-tools/src/lib.rs b/tools/aml-test-tools/src/lib.rs index 20b054d3..6cad0af9 100644 --- a/tools/aml-test-tools/src/lib.rs +++ b/tools/aml-test-tools/src/lib.rs @@ -25,7 +25,7 @@ use log::{error, trace}; use std::{ cell::SyncUnsafeCell, ffi::OsStr, - fmt::Debug, + fmt::{Debug, Formatter}, fs::File, io::{Read, Write}, panic::{AssertUnwindSafe, catch_unwind}, @@ -53,6 +53,19 @@ where Panicked, } +impl std::fmt::Debug for RunTestResult +where + T: Handler, +{ + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + RunTestResult::Pass(_) => "Pass", + RunTestResult::Failed(_, _) => "Failed", + RunTestResult::Panicked => "Panicked", + }) + } +} + /// The result of a test #[derive(Debug, PartialEq)] pub enum TestResult { diff --git a/tools/aml-tester/src/main.rs b/tools/aml-tester/src/main.rs index 7568e45c..13e3ae17 100644 --- a/tools/aml-tester/src/main.rs +++ b/tools/aml-tester/src/main.rs @@ -14,7 +14,11 @@ use acpi::Handler; use aml_test_tools::{ - handlers::{logging_handler::LoggingHandler, null_handler::NullHandler}, + handlers::{ + logging_handler::LoggingHandler, + null_handler::NullHandler, + sys_timer_handler::SystemTimerHandler, + }, new_interpreter, resolve_and_compile, result::ExpectedResult, @@ -300,5 +304,5 @@ fn find_tests(matches: &clap::ArgMatches) -> std::io::Result> { } fn new_handler() -> impl Handler { - LoggingHandler::new(NullHandler {}) + LoggingHandler::new(SystemTimerHandler::new(NullHandler {}, 1)) }