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

Break in nested try/catch/finally cause multiple panics #1900

Closed
addisoncrump opened this issue Mar 6, 2022 · 3 comments
Closed

Break in nested try/catch/finally cause multiple panics #1900

addisoncrump opened this issue Mar 6, 2022 · 3 comments
Assignees
Labels
bug Something isn't working execution Issues or PRs related to code execution help wanted Extra attention is needed vm Issues and PRs related to the Boa Virtual Machine.
Milestone

Comments

@addisoncrump
Copy link
Contributor

Describe the bug
Several panic variants caused by breaks in nested try/catch/finally. I have lumped them together as they appear to be caused by similar issues, but I have not yet identified a cause.

To Reproduce
This JavaScript crashes with thread 'main' panicked at 'stack was empty', boa_engine/src/vm/mod.rs:145:36

while (true) {
    try {
        try {
            break;
        } catch (e) {
        } finally {
        }
    } finally {
    }
}

This JavaScript crashes with thread 'main' panicked at 'index out of bounds: the len is 1 but the index is 25344', boa_engine/src/vm/mod.rs:396:39

while (true) {
    try {
        try {
            throw "h";
        } catch (e) {
            break;
        } finally {
        }
    } finally {
    }
}

This JavaScript crashes with thread 'main' panicked at 'attempt to subtract with overflow', boa_engine/src/vm/call_frame.rs:64:9

while (true) {
    try {
        try {
        } catch (e) {
        } finally {
            break;
        }
    } finally {
    }
}

Expected behavior
None of these should panic.

@addisoncrump addisoncrump added the bug Something isn't working label Mar 6, 2022
@Razican Razican added execution Issues or PRs related to code execution vm Issues and PRs related to the Boa Virtual Machine. labels Mar 14, 2022
@Razican Razican added this to the v0.15.0 milestone Mar 14, 2022
@tsutton
Copy link
Contributor

tsutton commented Apr 2, 2022

Additional information: it appears that with nested try ... finally blocks, the outer finally is skipped if there's a break or return:

 function f(){
    try {
        try {
	    return;
        } finally {
	console.log("inner")
        }
    } finally {
	console.log("outer")
    }
}

f();

Under node, this prints "inner" and "outer". Under boa (via cargo run), this just prints inner.

@Razican Razican modified the milestones: v0.15.0, v0.16.0 Jun 1, 2022
@addisoncrump
Copy link
Contributor Author

Error messages have changed but behaviour persists.

@Razican Razican modified the milestones: v0.16.0, v0.17.0 Sep 23, 2022
@Razican Razican added the help wanted Extra attention is needed label Oct 21, 2022
@nekevss nekevss self-assigned this Jan 16, 2023
bors bot pushed a commit that referenced this issue Feb 5, 2023
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel necessary.
--->

This Pull Request is meant to address #1900. While working on it, there was a decent amount of refactoring/restructuring. Initially, I had kept with the current approach of just keeping track of a kind and counter on the environment stack, especially because that kept the size of each stack entry to a minimum. 

I did, however, make a switch to having the opcode create the `EnvStackEntry` with a start address and exit address for a bit more precision.

It changes the following:

- Consolidates `loop_env_stack` and `try_env_stack` into one `EnvStackEntry` struct.
- Changes `Continue`, `Break`, `LoopStart`, `LoopContinue`, `FinallyStart`, `FinallyEnd` and various others. Most of this primarily revolves around the creating of `EnvStackEntry` and interacting with the `env_stack`. 
- Changes/updates the try-catch-finally, break and continue statement compilations as necessary
- Adds an `AbruptCompletionRecord` in place of the `finally_jump` vector.
- Adds some tests for try-catch-finally blocks with breaks.
@jedel1043
Copy link
Member

Closing since this is now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution help wanted Extra attention is needed vm Issues and PRs related to the Boa Virtual Machine.
Projects
Status: Done
Development

No branches or pull requests

5 participants