Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix backtracking on the topmost predicate triggering UB in run_module_predicate #2817

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

adri326
Copy link
Contributor

@adri326 adri326 commented Feb 5, 2025

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

?- 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.


There is only one detail left, which is that if the topmost predicate backtracks, run_module_predicate will exit while indicating success (when it really just failed).

I chose to leave it at that and document it as such, as this is a behavior that is somewhat relied on, notably when loading a file with a syntax error (you wouldn't want that to cause a panic). Instead, I'm catching any "escaping" errors in '$toplevel':'$repl'/0 to make it halt with error code 99.

…_predicate

Fixes mthom#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 mthom#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` :)
@adri326 adri326 force-pushed the fix-2815-run_module_predicate-backtrack branch from b637a63 to 22538a0 Compare February 5, 2025 23:43
@triska
Copy link
Contributor

triska commented Feb 5, 2025

(speechlessness)

@adri326
Copy link
Contributor Author

adri326 commented Feb 6, 2025

I couldn't find an easy way to tell apart throwing predicates from failing predicates in run_module_predicate, so I chose to instead have '$toplevel':'$repl'/0 catch the error and halt with a nonzero error code (here 99), that way there is something to diagnose off of, in case the user_output stream gets broken enough any other error printing predicate throws.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Machine::run_module_predicate triggers UB when a toplevel error is thrown
2 participants