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

rebis-dev: Fix UB in build_meta_predicate_clause #2764

Open
wants to merge 1 commit into
base: rebis-dev
Choose a base branch
from

Conversation

adri326
Copy link
Contributor

@adri326 adri326 commented Jan 9, 2025

This fixes #2763, where a logic bug in build_meta_predicate_clause makes it try to read out of bound in the heap, which, since it doesn't do bounds checking, triggers UB.

Before this PR, build_meta_predicate_clause would erronerously access the n-th argument of ':-'/2, as it was invoked with the address of the predicate itself, rather than that of the meta call that was found within the goals of that predicate. See #2763 for an example of this.

As I fixed the logic, I realized that when that function gets called, all the meta-predicate module: prefixes were already populated, so I am unsure what this function actually "builds".

This also seems to fix the segfault occasionally found in benchmarks; valgrind was able to pick that one up quite easily.

I don't know if that function is still needed, as making it return Default::default()
still passes all tests.

Nonetheless, this is now one less source of segfaults.

Fixes mthom#2763
@triska
Copy link
Contributor

triska commented Jan 9, 2025

Thank you a lot for such an impressive sequence of excellent contributions!

It seems this could be related to dynamic compilation of arguments that are known to refer to goals (due to meta-predicate declarations): Scryer Prolog is able to make calls of partially known goals faster by compiling them to point to clauses that are generated specifically to suit these goals.

For example:

:- use_module(library(diag)).
:- use_module(library(format)).
:- use_module(library(lists)).

test(X) :-
        call(pred, X).

pred(a).

Here, it is known that pred/1 (as opposed to any other goal) is called dynamically, and Scryer generates the following WAM instructions for test/1:

?- wam_instructions(test/1, Is),
   maplist(portray_clause, Is).
put_structure(pred,0,x(3)).
set_constant('$index_ptr'(29100)).
get_variable(x(2),1).
put_structure(:,2,x(1)).
set_constant(user).
set_value(x(3)).
execute(call,2).

Note the '$index_ptr' which points to the compiled body of pred/1:

?- inlined_instructions(29100, Is),
   maplist(portray_clause, Is).
get_constant(level(shallow),a,x(1)).
proceed.

As I understand it (@mthom please correct if anything is wrong here!), this is done to avoid goal expansions at run time, which would otherwise take place when pred/1 is dynamically invoked via call(pred, ...). Since the goal is known statically, a single iteration of goal expansion at compilation time is enough, and that goal-expanded version of the predicate can be directly called dynamically.

@adri326
Copy link
Contributor Author

adri326 commented Jan 9, 2025

Hmm, I'm unsure about that, since turning off that preprocessor step (by making it return Default::default()) doesn't result in any difference in the generated WAM instructions, nor does the fix in this PR.

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.

2 participants