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

src: fix potential segmentation fault in SQLite #53850

Merged
merged 1 commit into from
Jul 16, 2024

Conversation

tniessen
Copy link
Member

The Local<Value> returned from ColumnToValue() and ColumnNameToValue() may be empty (if a JavaScript exception is pending), in which case a segmentation fault may occur at the call sites, which do not check if the Local<Value> is empty. Fix this bug by returning early if an exception is pending (as indicated by the Local being empty).

In the long term, these functions should return MaybeLocal instead of Local, but this patch is supposed to be a minimal bug fix only.

The Local<Value> returned from ColumnToValue() and ColumnNameToValue()
may be empty (if a JavaScript exception is pending), in which case a
segmentation fault may occur at the call sites, which do not check if
the Local<Value> is empty. Fix this bug returning early if an exception
is pending (as indicated by the Local being empty).

In the long term, these functions should return MaybeLocal instead of
Local, but this patch is supposed to be a minimal bug fix only.
@tniessen tniessen added the sqlite Issues and PRs related to the SQLite subsystem. label Jul 14, 2024
@tniessen tniessen requested a review from cjihrig July 14, 2024 21:54
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jul 14, 2024
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 14, 2024
@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 14, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 14, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Is there an easy repro?

@tniessen
Copy link
Member Author

@anonrig The test case in #53851 segfaults without this PR :)

Comment on lines +444 to +446
if (key.IsEmpty()) return;
Local<Value> val = stmt->ColumnToValue(i);
if (val.IsEmpty()) return;
Copy link
Member

Choose a reason for hiding this comment

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

nit... is it worth combining these into a single if (key.IsEmpty() || val.IsEmpty()) return just to make things a bit more compact?

Copy link
Member Author

Choose a reason for hiding this comment

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

We usually fail fast when a JavaScript exception is pending to avoid unsafe calls into JavaScript in that state. It seems that ColumnToValue() does not call into JavaScript, but unless someone feels strongly, I'd err on the side of caution here.

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 16, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 16, 2024
@nodejs-github-bot nodejs-github-bot merged commit 0b1ff69 into nodejs:main Jul 16, 2024
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 0b1ff69

@tniessen tniessen added the experimental Issues and PRs related to experimental features. label Jul 17, 2024
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
The Local<Value> returned from ColumnToValue() and ColumnNameToValue()
may be empty (if a JavaScript exception is pending), in which case a
segmentation fault may occur at the call sites, which do not check if
the Local<Value> is empty. Fix this bug returning early if an exception
is pending (as indicated by the Local being empty).

In the long term, these functions should return MaybeLocal instead of
Local, but this patch is supposed to be a minimal bug fix only.

PR-URL: nodejs#53850
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
targos pushed a commit that referenced this pull request Jul 28, 2024
The Local<Value> returned from ColumnToValue() and ColumnNameToValue()
may be empty (if a JavaScript exception is pending), in which case a
segmentation fault may occur at the call sites, which do not check if
the Local<Value> is empty. Fix this bug returning early if an exception
is pending (as indicated by the Local being empty).

In the long term, these functions should return MaybeLocal instead of
Local, but this patch is supposed to be a minimal bug fix only.

PR-URL: #53850
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 30, 2024
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants