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.

Sign up for GitHub

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

Jump to bottom

"deserializer.readDouble()" results in an abort #37978

Closed
zyscoder opened this issue Mar 30, 2021 · 4 comments · Fixed by #38121
Closed

"deserializer.readDouble()" results in an abort #37978

zyscoder opened this issue Mar 30, 2021 · 4 comments · Fixed by #38121
Labels
confirmed-bug Issues with confirmed bugs. v8 module Issues and PRs related to the "v8" subsystem.

Comments

@zyscoder
Copy link

What steps will reproduce the bug?

Setup a node instance,

» node

and run the following javascript code.

new v8.Deserializer(new v8.Serializer().releaseBuffer()).readDouble()

Then an abort occurs.

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

This abort can always be triggered following the steps above.

What is the expected behavior?

If any error occurs, an exception or other similar error-reporting stuff should be thrown. There is no reason to abort the whole node process.

What do you see instead?

» node
> new v8.Deserializer(new v8.Serializer().releaseBuffer()).readDouble()
[1]    452627 segmentation fault (core dumped)  /path/to/node-v14.14.0/node

Additional information
@Ayase-252 Ayase-252 added the v8 module Issues and PRs related to the "v8" subsystem. label Mar 30, 2021
@EladKeyshawn EladKeyshawn mentioned this issue Apr 1, 2021
Closed
@EladKeyshawn
Copy link

Hey there ! 😄 I wanted to suggest a fix I'm not really sure about all the conventions
so I'd be happy if someone could check my PR: #38031

@aduh95 aduh95 added the confirmed-bug Issues with confirmed bugs. label Apr 6, 2021
@aduh95
Copy link
Contributor

aduh95 commented Apr 6, 2021

I can reproduce on master.

lazyparser pushed a commit to riscv-collab/v8 that referenced this issue Apr 6, 2021
@cjihrig
Fix ValueDeserializer::ReadDouble() bounds check
501482c 
If end_ is smaller than sizeof(double), the result would wrap
around, and lead to an invalid memory access.

Refs: nodejs/node#37978
Change-Id: Ibc8ddcb0c090358789a6a02f550538f91d431c1d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2801353
Reviewed-by: Marja Hölttä <[email protected]>
Commit-Queue: Marja Hölttä <[email protected]>
Cr-Commit-Position: refs/heads/master@{#73800}
@RaisinTen
Copy link
Contributor

Should this remain open anymore? The fix for this seems to have landed already in v8 at https://chromium-review.googlesource.com/c/v8/v8/+/2801353.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 6, 2021

I plan to open a PR to bring that commit into Node later today.

cjihrig added a commit to cjihrig/node that referenced this issue Apr 7, 2021
@cjihrig
deps: V8: cherry-pick 501482cbc704
50a26f4 
Original commit message:

    Fix ValueDeserializer::ReadDouble() bounds check

    If end_ is smaller than sizeof(double), the result would wrap
    around, and lead to an invalid memory access.

    Refs: nodejs#37978
    Change-Id: Ibc8ddcb0c090358789a6a02f550538f91d431c1d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2801353
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#73800}

Refs: v8/v8@501482cbc704
Fixes: nodejs#37978
@cjihrig cjihrig mentioned this issue Apr 7, 2021
Merged
cjihrig added a commit to cjihrig/node that referenced this issue Apr 8, 2021
@cjihrig
deps: V8: cherry-pick 501482cbc704
a7f7ed1 
Original commit message:

    Fix ValueDeserializer::ReadDouble() bounds check

    If end_ is smaller than sizeof(double), the result would wrap
    around, and lead to an invalid memory access.

    Refs: nodejs#37978
    Change-Id: Ibc8ddcb0c090358789a6a02f550538f91d431c1d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2801353
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#73800}

Refs: v8/v8@501482cbc704
Fixes: nodejs#37978
cjihrig added a commit to cjihrig/node that referenced this issue Apr 8, 2021
@cjihrig
test: add regression test for serdes readDouble()
8ef497c 
Refs: nodejs#37978
cjihrig added a commit to cjihrig/node that referenced this issue Apr 9, 2021
@cjihrig
test: add regression test for serdes readDouble()
e96773b 
Refs: nodejs#37978
PR-URL: nodejs#38121
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@cjihrig cjihrig closed this as completed in ca13f7a Apr 9, 2021
targos pushed a commit that referenced this issue May 1, 2021
@cjihrig @targos
deps: V8: cherry-pick 501482cbc704
8951c19 
Original commit message:

    Fix ValueDeserializer::ReadDouble() bounds check

    If end_ is smaller than sizeof(double), the result would wrap
    around, and lead to an invalid memory access.

    Refs: #37978
    Change-Id: Ibc8ddcb0c090358789a6a02f550538f91d431c1d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2801353
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#73800}

PR-URL: #38121
Fixes: #37978
Refs: v8/v8@501482cbc704
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this issue May 1, 2021
@cjihrig @targos
test: add regression test for serdes readDouble()
aa0d814 
Refs: #37978
PR-URL: #38121
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
danielleadams pushed a commit that referenced this issue May 8, 2021
@cjihrig @danielleadams
deps: V8: cherry-pick 501482cbc704
a6dba4d 
Original commit message:

    Fix ValueDeserializer::ReadDouble() bounds check

    If end_ is smaller than sizeof(double), the result would wrap
    around, and lead to an invalid memory access.

    Refs: #37978
    Change-Id: Ibc8ddcb0c090358789a6a02f550538f91d431c1d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2801353
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#73800}

PR-URL: #38121
Fixes: #37978
Refs: v8/v8@501482cbc704
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
danielleadams pushed a commit that referenced this issue May 8, 2021
@cjihrig @danielleadams
test: add regression test for serdes readDouble()
7162bea 
Refs: #37978
PR-URL: #38121
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. v8 module Issues and PRs related to the "v8" subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

deps: V8: cherry-pick 501482cbc704 cjihrig/node
6 participants
@cjihrig @Ayase-252 @aduh95 @EladKeyshawn @zyscoder @RaisinTen