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

worker: fix nullptr deref after MessagePort deser failure #25076

Closed
wants to merge 1 commit into from
Closed

worker: fix nullptr deref after MessagePort deser failure #25076

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

This would previously always have crashed when deserializing
a MessagePort fails, because there was always at least one
nullptr entry in the vector.

(Caught by @gireeshpunathil in #25061)

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

@addaleax
worker: fix nullptr deref after MessagePort deser failure
d0476f4 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.
@nodejs-github-bot
Copy link
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1970/pipeline

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 16, 2018
@addaleax addaleax added the worker Issues and PRs related to Worker support. label Dec 16, 2018
richardlau
richardlau reviewed Dec 16, 2018
View reviewed changes
src/node_messaging.cc
@@ -90,7 +90,8 @@ MaybeLocal<Value> Message::Deserialize(Environment* env,
if (ports[i] == nullptr) {
for (MessagePort* port : ports) {
// This will eventually release the MessagePort object itself.
port->Close();
if (port != nullptr)
Copy link
Member

Choose a reason for hiding this comment

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

Does the enclosing for loop need to loop through all ports or just up to (but not including since ports[i] == nullptr) i?

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau Yes, I think that should work too … should we optimize here? This effectively only occurs during .terminate(), and I don’t think it’s a typical case to pass more than one MessagePort per Message anyway?

Copy link
Member

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯ I'll leave it to your judgement.

This was referenced Dec 17, 2018
Closed
Closed
@Trott
Copy link
Member

Trott commented Dec 17, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/19625/ ✔️

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 17, 2018
@danbev
Copy link
Contributor

danbev commented Dec 19, 2018

Landed in e1ab457.

@danbev danbev closed this Dec 19, 2018
danbev pushed a commit that referenced this pull request Dec 19, 2018
@addaleax @danbev
worker: fix nullptr deref after MessagePort deser failure
e1ab457 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.

PR-URL: #25076
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@joyeecheung joyeecheung mentioned this pull request Dec 19, 2018
Closed
@addaleax addaleax deleted the messageport-deserialize-nullptr branch December 19, 2018 16:25
MylesBorins pushed a commit that referenced this pull request Dec 25, 2018
@addaleax @MylesBorins
worker: fix nullptr deref after MessagePort deser failure
4f28da8 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.

PR-URL: #25076
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 25, 2018
Merged
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@refack
worker: fix nullptr deref after MessagePort deser failure
2fdb78c 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.

PR-URL: nodejs#25076
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
@addaleax @BethGriggs
worker: fix nullptr deref after MessagePort deser failure
a7e1daf 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.

PR-URL: #25076
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@addaleax @BethGriggs
worker: fix nullptr deref after MessagePort deser failure
d6b7caf 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.

PR-URL: #25076
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Jul 2, 2019
@addaleax @BethGriggs
worker: fix nullptr deref after MessagePort deser failure
6d39161 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.

PR-URL: #25076
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
BethGriggs pushed a commit that referenced this pull request Jul 17, 2019
@addaleax @BethGriggs
worker: fix nullptr deref after MessagePort deser failure
14090b5 
This would previously always have crashed when deserializing
a `MessagePort` fails, because there was always at least one
`nullptr` entry in the vector.

PR-URL: #25076
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
BethGriggs added a commit that referenced this pull request Jul 17, 2019
@BethGriggs
2019-05-28, Version 10.16.1 'Dubnium' (LTS)
4d0e87c 
Notable changes:

- **stream**: do not unconditionally call `\_read()` on `resume()` (Anna Henningsen) [#26965](#26965)
- **worker**: fix nullptr deref after MessagePort deser failure (Anna Henningsen) [#25076](#25076)
@BethGriggs BethGriggs mentioned this pull request Jul 17, 2019
Merged
BethGriggs added a commit that referenced this pull request Jul 26, 2019
@BethGriggs
2019-07-30, Version 10.16.1 'Dubnium' (LTS)
2d18f84 
Notable changes:

- **deps**: upgrade openssl sources to 1.1.1c (Sam Roberts) [#28212](#28212)
- **stream**: do not unconditionally call `\_read()` on `resume()` (Anna Henningsen) [#26965](#26965)
- **worker**: fix nullptr deref after MessagePort deser failure (Anna Henningsen) [#25076](#25076)

PR-URL: #28731
BethGriggs added a commit that referenced this pull request Jul 30, 2019
@BethGriggs
2019-07-30, Version 10.16.1 'Dubnium' (LTS)
fe32fe8 
Notable changes:

- **deps**: upgrade openssl sources to 1.1.1c (Sam Roberts) [#28212](#28212)
- **stream**: do not unconditionally call `\_read()` on `resume()` (Anna Henningsen) [#26965](#26965)
- **worker**: fix nullptr deref after MessagePort deser failure (Anna Henningsen) [#25076](#25076)

PR-URL: #28731
BethGriggs added a commit that referenced this pull request Jul 31, 2019
@BethGriggs
2019-07-31, Version 10.16.1 'Dubnium' (LTS)
a395299 
Notable changes:

- **deps**: upgrade openssl sources to 1.1.1c (Sam Roberts) [#28212](#28212)
- **stream**: do not unconditionally call `\_read()` on `resume()` (Anna Henningsen) [#26965](#26965)
- **worker**: fix nullptr deref after MessagePort deser failure (Anna Henningsen) [#25076](#25076)

PR-URL: #28731
BethGriggs added a commit that referenced this pull request Jul 31, 2019
@BethGriggs
2019-07-31, Version 10.16.1 'Dubnium' (LTS)
9d37013 
Notable changes:

- **deps**: upgrade openssl sources to 1.1.1c (Sam Roberts) [#28212](#28212)
- **stream**: do not unconditionally call `\_read()` on `resume()` (Anna Henningsen) [#26965](#26965)
- **worker**: fix nullptr deref after MessagePort deser failure (Anna Henningsen) [#25076](#25076)

PR-URL: #28731
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@richardlau richardlau richardlau approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
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++. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@addaleax @nodejs-github-bot @Trott @danbev @jasnell @richardlau @gireeshpunathil