From 22538a05beff127903f2be6813b1facdf51baa07 Mon Sep 17 00:00:00 2001 From: Emilie Burgun Date: Thu, 6 Feb 2025 00:16:20 +0100 Subject: [PATCH 1/3] Fix backtracking on the topmost predicate triggering UB in run_module_predicate Fixes #2815, see that issue for my investigation. This is a one-line fix that I'm quite proud of :) If the topmost query for `run_module_predicate` needs to backtrack, then before this commit, one of the following two things may happen: - A dangling OrFrame is read at stack offset 0 - An AndFrame was at stack offset 0 would be read as an OrFrame This can be seen by either calling `run_module_predicate` with a throwing predicate (encountering the second scenario) or a failing predicate (encountering the first scenario), or by running the following in the REPL, which triggers a `throw/1` within the error handler, propagating it all the way up (and encountering the second scenario): ```prolog ?- current_output(S), open(stream(S), write, S0, [type(binary)]). ``` Currently, `Stack` is not equipped with tools to detect this incorrect behavior, so it would instead try to read an OrFrame at offset 0, which triggers UB, since transmuting between AndFramePrelude and OrFramePrelude isn't legal. In practice, since `AndFramePrelude` is smaller, the later fields of `OrFramePrelude` would read from the cells following the `AndFramePrelude`, and would contain nonsensical data, triggering the panic that led to my investigation in #2815 and that is fairly reliable to witness. Surprisingly, this wouldn't happen with `run_query`, which led me to look at how they operate differently. It turns out that `run_query` inserts an OrFrame at offset 0, which covers both problematic scenarios. The fix is thus to simply add a call to `Machine::allocate_stub_choice_point` in `run_module_predicate` :) --- src/machine/lib_machine/mod.rs | 2 +- src/machine/mod.rs | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/machine/lib_machine/mod.rs b/src/machine/lib_machine/mod.rs index 24f22fe7..be5c6008 100644 --- a/src/machine/lib_machine/mod.rs +++ b/src/machine/lib_machine/mod.rs @@ -544,7 +544,7 @@ impl Machine { self.run_module_predicate(atom!("loader"), (atom!("consult_stream"), 2)); } - fn allocate_stub_choice_point(&mut self) { + pub(crate) fn allocate_stub_choice_point(&mut self) { // NOTE: create a choice point to terminate the dispatch_loop // if an exception is thrown. diff --git a/src/machine/mod.rs b/src/machine/mod.rs index a62c0caa..cbd99c53 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -279,6 +279,7 @@ impl Machine { if let Some(module) = self.indices.modules.get(&module_name) { if let Some(code_index) = module.code_dir.get(&key) { let p = code_index.local().unwrap(); + self.allocate_stub_choice_point(); self.machine_st.cp = BREAK_FROM_DISPATCH_LOOP_LOC; self.machine_st.p = p; @@ -1264,3 +1265,25 @@ impl Machine { } } } + +#[cfg(test)] +mod tests { + use super::config::*; + use super::*; + + #[test] + fn test_run_module_predicate_throw() { + let mut machine = MachineBuilder::default() + .with_toplevel( + r#" + :- module('$toplevel', []). + repl :- throw(kaboom). + "#, + ) + .build(); + + let query = machine.run_module_predicate(atom!("$toplevel"), (atom!("repl"), 0)); + + assert_eq!(query, std::process::ExitCode::SUCCESS); + } +} From d4bf52e82c131882fbc2f7fd213ea39548ed78b6 Mon Sep 17 00:00:00 2001 From: Emilie Burgun Date: Thu, 6 Feb 2025 10:37:54 +0100 Subject: [PATCH 2/3] Disable test_run_module_predicate_throw under miri and support rustc < 1.83 --- src/machine/mod.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/machine/mod.rs b/src/machine/mod.rs index cbd99c53..bfa4f7b0 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -1272,18 +1272,17 @@ mod tests { use super::*; #[test] + #[cfg_attr(miri, ignore)] fn test_run_module_predicate_throw() { let mut machine = MachineBuilder::default() .with_toplevel( r#" - :- module('$toplevel', []). - repl :- throw(kaboom). - "#, + :- module('$toplevel', []). + repl :- throw(kaboom). + "#, ) .build(); - let query = machine.run_module_predicate(atom!("$toplevel"), (atom!("repl"), 0)); - - assert_eq!(query, std::process::ExitCode::SUCCESS); + machine.run_module_predicate(atom!("$toplevel"), (atom!("repl"), 0)); } } From e68ac8347f998827c8dca651954c5527d4936c77 Mon Sep 17 00:00:00 2001 From: Emilie Burgun Date: Thu, 6 Feb 2025 13:26:16 +0100 Subject: [PATCH 3/3] Document run_module_predicate and handle critical failure in toplevel.pl --- src/machine/mod.rs | 6 ++++++ src/toplevel.pl | 13 +++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/machine/mod.rs b/src/machine/mod.rs index bfa4f7b0..9a6c50d7 100644 --- a/src/machine/mod.rs +++ b/src/machine/mod.rs @@ -271,6 +271,11 @@ impl Machine { .unwrap() } + /// Runs the predicate `key` in `module_name` until completion. + /// Siltently ignores failure, thrown errors and choice points. + /// + /// Consider using [`Machine::run_query`] if you wish to handle + /// predicates that may fail, leave a choice point or throw. pub(crate) fn run_module_predicate( &mut self, module_name: Atom, @@ -279,6 +284,7 @@ impl Machine { if let Some(module) = self.indices.modules.get(&module_name) { if let Some(code_index) = module.code_dir.get(&key) { let p = code_index.local().unwrap(); + // Leave a halting choice point to backtrack to in case the predicate fails or throws. self.allocate_stub_choice_point(); self.machine_st.cp = BREAK_FROM_DISPATCH_LOOP_LOC; diff --git a/src/toplevel.pl b/src/toplevel.pl index b43ac3cb..0727aee0 100644 --- a/src/toplevel.pl +++ b/src/toplevel.pl @@ -29,6 +29,19 @@ load_scryerrc :- ). '$repl' :- + catch( + start_repl, + _, + % Something bad enough happened that the REPL itself threw an error. + % This can be caused by a broken user_output stream, so we cannot + % print an error. + % + % The best we can do now is halt with an error code, + % so that users can try to diagnose the issue: + halt(99) + ). + +start_repl :- asserta('$toplevel':started), raw_argv(Args0), ( append(Args1, ["--"|_], Args0) ->