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

v8: fix native serdes constructors #36549

Closed

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Dec 17, 2020

This removes the indirection added by the class Serializer extends _Serializer {} by utilising THROW_ERR_CONSTRUCT_CALL_REQUIRED from node_errors.h, which is used by MessageChannel to avoid the same issue.


This also makes it so that a new Serializer() or new Deserializer(buffer) call with a poisoned Array.prototype[Symbol.iterator] works fine.


This also prevents:

Object.getPrototypeOf(require('v8').Serializer)()

from crashing the node process.

Related Issues

Fixes: #13326

Refs: #13541

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 17, 2020
@ExE-Boss ExE-Boss force-pushed the lib/v8/fix-native-serdes-constructors branch from 115fb21 to 3a8ce4d Compare December 17, 2020 11:21
@Trott
Copy link
Member

Trott commented Dec 17, 2020

This doesn't change the error name (it's still a TypeError) or the error message but does introduce a code where there wasn't one before. This is a good thing and does not seem like a breaking change to me. (It's conceivable that someone is checking for the absence of a code, but...seems unlikely.) I'm inclined to consider the code a new feature and mark this semver-minor. That will get it widely distributed quickly except maybe in maintenance LTS lines, which I think seems about right. Putting my reasoning here in case anyone disagrees and wants to change it. I don't feel strongly about it. I just want to make sure we think about it.

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 17, 2020
@Trott
Copy link
Member

Trott commented Dec 17, 2020

The Fixes points to an issue that's already closed with another fix in place. I guess this replaces the original fix?

@Trott Trott added v8 module Issues and PRs related to the "v8" subsystem. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 17, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2020
@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 17, 2020
@addaleax
Copy link
Member

addaleax commented Dec 17, 2020

I’m still adding the semver-major label because of the read-only prototype change. Sorry, this matches existing behavior. That should be fine then 👍

@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 17, 2020
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 21, 2020
@addaleax addaleax added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 22, 2020
@github-actions github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 22, 2020
@github-actions
Copy link
Contributor

Commit Queue failed
- Loading data for nodejs/node/pull/36549
✔  Done loading data for nodejs/node/pull/36549
----------------------------------- PR info ------------------------------------
Title      v8: Fix native `serdes` constructors (#36549)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     ExE-Boss:lib/v8/fix-native-serdes-constructors -> nodejs:master
Labels     C++, author ready, semver-minor, v8 module
Commits    1
 - v8: Fix native `serdes` constructors
Committers 1
 - ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/36549
Fixes: https://github.com/nodejs/node/issues/13326
Refs: https://github.com/nodejs/node/pull/13541
Reviewed-By: Rich Trott 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36549
Fixes: https://github.com/nodejs/node/issues/13326
Refs: https://github.com/nodejs/node/pull/13541
Reviewed-By: Rich Trott 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-12-17T14:15:46Z: https://ci.nodejs.org/job/node-test-pull-request/34989/
- Querying data for job/node-test-pull-request/34989/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Thu, 17 Dec 2020 11:10:14 GMT
   ✔  Approvals: 2
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/36549#pullrequestreview-554665625
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36549#pullrequestreview-554904952
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 36549
From https://github.com/nodejs/node
 * branch                  refs/pull/36549/merge -> FETCH_HEAD
✔  Fetched commits as 656ce920a332..3a8ce4d8496c
--------------------------------------------------------------------------------
[master d841a5e21c] v8: Fix native `serdes` constructors
 Author: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
 Date: Thu Dec 17 12:10:00 2020 +0100
 3 files changed, 23 insertions(+), 16 deletions(-)
   ✔  Patches applied
--------------------------------------------------------------------------------
   ⚠  Found Fixes: https://github.com/nodejs/node/issues/13326, skipping..
   ⚠  Found Refs: https://github.com/nodejs/node/pull/13541, skipping..
--------------------------------- New Message ----------------------------------
v8: Fix native `serdes` constructors

Fixes: #13326

Refs: #13541

PR-URL: #36549
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

[master 863ac31b5a] v8: Fix native serdes constructors
Author: ExE Boss 3889017+ExE-Boss@users.noreply.github.com
Date: Thu Dec 17 12:10:00 2020 +0100
3 files changed, 23 insertions(+), 16 deletions(-)
✖ 863ac31b5a48f8025406287dad9d7d1d979edcad
✔ 1:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 5:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✖ 0:5 First word after subsystem(s) in title should be lowercase. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/438086488

@addaleax
Copy link
Member

Right, sorry, the commit message :)

Landed in d90fa19

@addaleax addaleax closed this Dec 22, 2020
addaleax pushed a commit that referenced this pull request Dec 22, 2020
Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@ExE-Boss ExE-Boss deleted the lib/v8/fix-native-serdes-constructors branch December 23, 2020 09:26
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
danielleadams added a commit that referenced this pull request Jan 12, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 12, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 13, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 13, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 13, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 14, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603)
* crypto:
  * implement basic secure heap support (James M Snell) [#36779](#36779)
  * fixup bug in keygen error handling (James M Snell) [#36779](#36779)
  * introduce X509Certificate API (James M Snell) [#36804](#36804)
  * implement randomuuid (James M Snell) [#36729](#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) [#36793](#36793)
  * add dnlup to collaborators (Daniele Belardi) [#36849](#36849)
  * add panva to collaborators (Filip Skokan) [#36802](#36802)
  * add yashLadha to collaborator (Yash Ladha) [#36666](#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291)
* v8:
  * fix native  constructors (ExE Boss) [#36549](#36549)
danielleadams added a commit that referenced this pull request Jan 14, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) (#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) (#36603)
* crypto:
  * implement basic secure heap support (James M Snell) (#36779)
  * fixup bug in keygen error handling (James M Snell) (#36779)
  * introduce X509Certificate API (James M Snell) (#36804)
  * implement randomuuid (James M Snell) (#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) (#36793)
  * add dnlup to collaborators (Daniele Belardi) (#36849)
  * add panva to collaborators (Filip Skokan) (#36802)
  * add yashLadha to collaborator (Yash Ladha) (#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) (#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) (#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) (#34291)
* v8:
  * fix native  constructors (ExE Boss) (#36549)
danielleadams added a commit that referenced this pull request Jan 15, 2021
PR-URL: #36889

Notable changes:

* child_process:
  * add 'overlapped' stdio flag (Thiago Padilha) (#29412)
  * support AbortSignal in fork (Benjamin Gruenbaum) (#36603)
* crypto:
  * implement basic secure heap support (James M Snell) (#36779)
  * fixup bug in keygen error handling (James M Snell) (#36779)
  * introduce X509Certificate API (James M Snell) (#36804)
  * implement randomuuid (James M Snell) (#36729)
* doc:
  * update release key for Danielle Adams (Danielle Adams) (#36793)
  * add dnlup to collaborators (Daniele Belardi) (#36849)
  * add panva to collaborators (Filip Skokan) (#36802)
  * add yashLadha to collaborator (Yash Ladha) (#36666)
* http:
  * set lifo as the default scheduling strategy in Agent (Matteo Collina) (#36685)
* net:
  * support abortSignal in server.listen (Nitzan Uziely) (#36623)
* process:
  * add direct access to rss without iterating pages (Adrien Maret) (#34291)
* v8:
  * fix native  constructors (ExE Boss) (#36549)
@ExE-Boss ExE-Boss changed the title v8: Fix native serdes constructors v8: fix native serdes constructors Jan 17, 2021
targos pushed a commit that referenced this pull request May 1, 2021
Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021
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++. semver-minor PRs that contain new features and should be released in the next minor version. v8 module Issues and PRs related to the "v8" subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[8.0.0] v8.Serializer crashes on bad input
7 participants