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` :)

diff --git a/src/machine/lib_machine/mod.rs b/src/machine/lib_machine/mod.rs
index 24f22fe78ad701ba4ae998aed33bd426f81b0d77..be5c6008e93df711d9fc5a79b7a7b4e45b419e50 100644 (file)
--- 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 a62c0caa8e79fc6db41d9b9a5bf7e5526800a809..cbd99c531a31ab864533bac8e86abfb1d5ebd31a 100644 (file)
--- 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);
+    }
+}