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

Throwing in sqlite filter function crashes process #56890

Open
jasnell opened this issue Feb 3, 2025 · 7 comments · May be fixed by #56903
Open

Throwing in sqlite filter function crashes process #56890

jasnell opened this issue Feb 3, 2025 · 7 comments · May be fixed by #56903
Labels
sqlite Issues and PRs related to the SQLite subsystem.

Comments

@jasnell
Copy link
Member

jasnell commented Feb 3, 2025

Version

current

Platform

all

Subsystem

sqlite

What steps will reproduce the bug?

  database2.applyChangeset(session.changeset(), {
    filter() { throw new Error('boom'); }
  });

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

Error is thrown

What do you see instead?

Process crashes

Additional information

No response

@jasnell
Copy link
Member Author

jasnell commented Feb 3, 2025

/cc @cjihrig

@jasnell
Copy link
Member Author

jasnell commented Feb 3, 2025

Fixing the issue will necessitate refactoring the xFilter static function used in the DatabaseSync::ApplyChangeset method in node_sqlite

@cjihrig
Copy link
Contributor

cjihrig commented Feb 3, 2025

I'll take a look, but cc @louwers who worked on the session extension.

@louwers
Copy link
Contributor

louwers commented Feb 3, 2025

I can take a look.

@jasnell
Copy link
Member Author

jasnell commented Feb 3, 2025

@louwers ... the key issue, if it's not obvious, is that the filter callback function is using ToLocalChecked() within the filter callback. If the callback throws, then the returned value from the callback will be empty causing ToLocalChecked to trigger the abort. Instead the thrown error should be allowed to propagate. However, because of the way the xFilter callback is set up it's not immediately obvious what should change. We essentially need a way of allowing that API to return an error code that means "Hey, v8 scheduled an error, throw it instead"

@cjihrig
Copy link
Contributor

cjihrig commented Feb 3, 2025

We essentially need a way of allowing that API to return an error code that means "Hey, v8 scheduled an error, throw it instead"

Something like #56787 might work if it can ever get merged.

@louwers
Copy link
Contributor

louwers commented Feb 3, 2025

I created a PR: #56903

@cjihrig cjihrig added the sqlite Issues and PRs related to the SQLite subsystem. label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants