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: panic, unwrap to unwrap_or #739

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

peko-thunder
Copy link

I created a PR in deno_core to correct the issue.

#744

Sometimes panic occurs with unwrap().
So I set the default value.

Do I need to write test code?
I'm reading the code right now, it's going to take some time.

@CLAassistant
Copy link

CLAassistant commented May 15, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Thanks! Could you add a test? Maybe use unwrap_or_else instead?

@peko-thunder
Copy link
Author

@ry
I will add a test to integration_test in testing/lib.rs.
Test that there is no panic when executing code like issue.

@peko-thunder
Copy link
Author

@ry
I added an integration test.
I don't use unwrap_or_else(), is that a problem?

@bartlomieju
Copy link
Member

@peko-thunder can you please run cargo fmt on your branch?

@0f-0b
Copy link
Contributor

0f-0b commented May 20, 2024

Instead of concealing the bug like this, can we fix ArrayPrototypePush so that it does not accidentally call user code?

@bartlomieju
Copy link
Member

@0f-0b can you elaborate more on the suggested solution?

@peko-thunder
Copy link
Author

@bartlomieju
I’m sorry. I’ll be careful next time.

@0f-0b
Copy link
Contributor

0f-0b commented May 20, 2024

can you elaborate more on the suggested solution?

The ultimate cause of this bug is that Array.prototype.push calls the [[Set]] internal method on the receiver (step 5.a) which executes the user-defined setter, and that ArrayPrototypePush is currently just uncurryThis(Array.prototype.push). My suggestion is to change ArrayPrototypePush to a custom implementation that calls [[DefineOwnProperty]] to avoid executing user code. A precedent of providing a custom implemention to avoid unintentional side-effects is ArrayPrototypeToString.

@bartlomieju
Copy link
Member

@devsnek what do you think of the approach above?

@devsnek
Copy link
Member

devsnek commented May 21, 2024

assuming you are referring to this

ArrayPrototypePush(error.__callSiteEvals, callSite);

i think applying both approaches make sense. runtime internal js should have a way to push an element to an array without the prototype breaking things, and runtime internal rust should probably be able to cope with weird things happening in js without crashing, unless the weird thing is truly a case where the runtime cannot safely continue execution.

@peko-thunder
Copy link
Author

Thanks for the suggestion.
I may not understand it yet, but I confirmed that if I handle the push() pseudo, I won't get the error.
I'll do some more testing.

  primordials.ArrayPrototypePush = (thisArray, ...variableArgs) =>
    thisArray = [...thisArray, ...variableArgs];

@peko-thunder
Copy link
Author

Confirmed that all tests pass.
If you see a problem, point it out.

  primordials.ArrayPrototypePush = (thisArray, ...variableArgs) => {
    const args = [...variableArgs];
    for (let i = 0; i < args.length; i++) {
      ObjectDefineProperty(thisArray, thisArray.length, {
        value: args[i],
        enumerable: true,
        writable: true,
        configurable: true,
      });
    }
    return thisArray.length;
  };

Copy link
Contributor

@0f-0b 0f-0b left a comment

Choose a reason for hiding this comment

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

The panic may still happen if user code overwrites __callSiteEvals. I will investigate if it is possible to hide __callSiteEvals from user code later.

throw Object.defineProperty(new Error(), "__callSiteEvals", { value: [0] });

ObjectDefineProperty is really slow, but we can always replace ArrayPrototypePush(array, value) with array[array.length] = value in affected code. The latter is not only much faster, but also makes it explicit that [[Set]] is called.

core/00_primordials.js Outdated Show resolved Hide resolved
core/00_primordials.js Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented May 23, 2024

The panic may still happen if user code overwrites __callSiteEvals.

Yeah I would suggest keeping the rust unwrap_or in this PR as well.

I will investigate if it is possible to hide __callSiteEvals from user code later.

For hiding __callSiteEvals I think you could replace it with a WeakMap that is shared with the rust code. Using a v8 private symbol is a fun trick as well but it requires a transmute :(

@0f-0b
Copy link
Contributor

0f-0b commented May 23, 2024

Why wasn't v8::Exception::get_stack_trace used here?

Edit: I see that a lot of useful information is only available to the prepareStackTrace callback. Was this the only reason?

Comment on lines +490 to +492
primordials.ArrayPrototypePush = (thisArray, ...args) => {
for (let i = 0; i < args.length; i++) {
ObjectDefineProperty(thisArray, thisArray.length, {
Copy link
Member

Choose a reason for hiding this comment

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

Given the fix in #754, do we really want to change this?

@0f-0b @devsnek

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.

6 participants