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 rare crash caused by get method of proxy object #5129

Merged
merged 1 commit into from Mar 13, 2024

Conversation

matetokodi
Copy link
Contributor

This fixes #5101
In rare cases the proxy object could get used after being incorrectly removed by the gc

Add stack checks to the start of all function calls

A Regression test is not included because I was unable to reproduce the crash with code that would be readable / easy to understand, only with a marginally simplified version of the example code given in the original issue, and @zherczeg advised against including it:

async function f0() {
    function f6() {
        return f0;
    }
    var proxy_handler = {
        "get": f0,
    };

    f0.__proto__ = new Proxy(f6, proxy_handler);
    var v12 = f0();
    return f0;
}
f0();

However I can include it anyways if requested.

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code changes look good to me, but I do think that the issue-reproducing test case should be included in the commit / test suite. A regression test does not have to be nice or easy-to-understand (it should be minimal though). Its purpose is to avoid the problem creeping back into the code base. (BTW, the test case is not big at all.)

@matetokodi
Copy link
Contributor Author

The code changes look good to me, but I do think that the issue-reproducing test case should be included in the commit / test suite.

Alright, I have included the regression test.

This fixes jerryscript-project#5101
In rare cases the proxy object could get used after
being incorrectly removed by the gc

Add stack checks to the start of all function calls

JerryScript-DCO-1.0-Signed-off-by: Máté Tokodi mate.tokodi@szteszoftver.hu
Copy link
Member

@akosthekiss akosthekiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@akosthekiss akosthekiss merged commit cefd391 into jerryscript-project:master Mar 13, 2024
26 checks passed
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.

SEGV ./jerry-core/ecma/base/ecma-helpers.c:238:58
3 participants