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

Add max recursion depth check to avoid stack overflow during statement evaluation #122

Merged
merged 3 commits into from
Jul 19, 2023

Conversation

kvnxiao
Copy link
Contributor

@kvnxiao kvnxiao commented Jul 19, 2023

E.g.

f(x) = f(x) + 1

Before:

kalker
Type 'help' for instructions.
>> f(x) = f(x) + 1
>> f(1)

thread 'main' has overflowed its stack
fatal runtime error: stack overflow

After:

kalker
Type 'help' for instructions.
>> f(x) = f(x) + 1
>> f(1)
Operation recursed too deeply.
>> exit

@kvnxiao
Copy link
Contributor Author

kvnxiao commented Jul 19, 2023

I'm currently using this as a bandaid solution for my use case. Let me know if you think there's a better way this can be structured and represented in the code.

@PaddiM8
Copy link
Owner

PaddiM8 commented Jul 19, 2023

Great! You'll need to decrement it as well though, since the stack won't overflow if you go in and out of recursions a lot. To make that work you'd probably need to increment right before function calls and decrement right after. Easiest would probably be to put that in eval_expr in the match arm for FnCall.

@kvnxiao
Copy link
Contributor Author

kvnxiao commented Jul 19, 2023

Would it make sense to only increment / decrement the depth counter within this part of the FnCall code?

    match stmt_definition {
        Some(Stmt::FnDecl(_, arguments, fn_body)) => {
// ... <somewhere here in this body?>

Or did you mean something like:

// fn eval_expr(...)
    match expr {
		// ...
        Expr::FnCall(identifier, expressions) => {
            context.recursion_depth += 1;
            let res = eval_fn_call_expr(context, identifier, expressions, unit);
            context.recursion_depth -= 1;
            res
        }

@kvnxiao
Copy link
Contributor Author

kvnxiao commented Jul 19, 2023

I updated the PR code, let me know if you think there's other things worth moving around

@PaddiM8
Copy link
Owner

PaddiM8 commented Jul 19, 2023

Looks good, thanks!

@PaddiM8 PaddiM8 merged commit 28b6e42 into PaddiM8:master Jul 19, 2023
@kvnxiao kvnxiao deleted the prevent-stack-overflow branch July 19, 2023 23:05
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