src: do not unnecessarily re-assign uv handle data by addaleax · Pull Request #31696 · nodejs/node

addaleax · 2020-02-08T16:06:34Z

a555be2 re-assigned async_.data to indicate success
or failure of the constructor. As the HandleWrap implementation
uses that field to access the HandleWrap instance from the
libuv handle, this introduced two issues:

It implicitly assumed that casting
MessagePort* → void* → HandleWrap* would be valid.
It made the HandleWrap::OnClose() function fail with a
nullptr dereference if the constructor did fail.

In particular, the second issue made
test/parallel/test-worker-cleanexit-with-moduleload.js crash at
least once in CI.

Since re-assigning async_.data isn’t actually necessary here
(only a leftover from earlier versions of that commit), fix this by
using a local variable instead, and add a CHECK that provides better
error messages for this type of issue in the future.

Refs: #31605

Example failure: https://ci.nodejs.org/job/node-test-commit-linux-containered/17944/nodes=ubuntu1804_sharedlibs_openssl111_x64/testReport/junit/(root)/test/parallel_test_worker_cleanexit_with_moduleload/

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

a555be2 re-assigned `async_.data` to indicate success or failure of the constructor. As the `HandleWrap` implementation uses that field to access the `HandleWrap` instance from the libuv handle, this introduced two issues: - It implicitly assumed that casting `MessagePort*` → `void*` → `HandleWrap*` would be valid. - It made the `HandleWrap::OnClose()` function fail with a `nullptr` dereference if the constructor did fail. In particular, the second issue made test/parallel/test-worker-cleanexit-with-moduleload.js` crash at least once in CI. Since re-assigning `async_.data` isn’t actually necessary here (only a leftover from earlier versions of that commit), fix this by using a local variable instead, and add a `CHECK` that provides better error messages for this type of issue in the future. Refs: nodejs#31605

nodejs-github-bot · 2020-02-08T16:09:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29022/

nodejs-github-bot · 2020-02-08T17:41:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29024/ (:white_check_mark:)

addaleax · 2020-02-10T16:29:30Z

Landed in f938cbd

a555be2 re-assigned `async_.data` to indicate success or failure of the constructor. As the `HandleWrap` implementation uses that field to access the `HandleWrap` instance from the libuv handle, this introduced two issues: - It implicitly assumed that casting `MessagePort*` → `void*` → `HandleWrap*` would be valid. - It made the `HandleWrap::OnClose()` function fail with a `nullptr` dereference if the constructor did fail. In particular, the second issue made test/parallel/test-worker-cleanexit-with-moduleload.js` crash at least once in CI. Since re-assigning `async_.data` isn’t actually necessary here (only a leftover from earlier versions of that commit), fix this by using a local variable instead, and add a `CHECK` that provides better error messages for this type of issue in the future. Refs: #31605 PR-URL: #31696 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

codebytere · 2020-03-15T16:20:36Z

@addaleax if this is something that should be in v12.x, could you please backport it there? There are a nest of conflicts and i want to be extra careful 😅

a555be2 re-assigned `async_.data` to indicate success or failure of the constructor. As the `HandleWrap` implementation uses that field to access the `HandleWrap` instance from the libuv handle, this introduced two issues: - It implicitly assumed that casting `MessagePort*` → `void*` → `HandleWrap*` would be valid. - It made the `HandleWrap::OnClose()` function fail with a `nullptr` dereference if the constructor did fail. In particular, the second issue made test/parallel/test-worker-cleanexit-with-moduleload.js` crash at least once in CI. Since re-assigning `async_.data` isn’t actually necessary here (only a leftover from earlier versions of that commit), fix this by using a local variable instead, and add a `CHECK` that provides better error messages for this type of issue in the future. Refs: nodejs#31605 PR-URL: nodejs#31696 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

a555be2 re-assigned `async_.data` to indicate success or failure of the constructor. As the `HandleWrap` implementation uses that field to access the `HandleWrap` instance from the libuv handle, this introduced two issues: - It implicitly assumed that casting `MessagePort*` → `void*` → `HandleWrap*` would be valid. - It made the `HandleWrap::OnClose()` function fail with a `nullptr` dereference if the constructor did fail. In particular, the second issue made test/parallel/test-worker-cleanexit-with-moduleload.js` crash at least once in CI. Since re-assigning `async_.data` isn’t actually necessary here (only a leftover from earlier versions of that commit), fix this by using a local variable instead, and add a `CHECK` that provides better error messages for this type of issue in the future. Refs: #31605 PR-URL: #31696 Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2020-04-28T16:23:06Z

@targos … this seems to have ended up on v12.x-staging before the v12.16.3 release, but not in the release itself. It fixes a segfault that is now occurring with v12.16.3 – is there anything I could have done differently here to prevent that?

(I do know that I kind of dropped the ball on backporting this, but as you noticed, that was only necessary because other commits hadn’t landed yet, so I don’t think it would have made a difference?)

targos · 2020-04-28T17:03:10Z

@addaleax this wasn't on v12.x-staging before the release. I just updated the branch right after it.

is there anything I could have done differently here to prevent that?

I think the most reliable way would have been to label the original PR as backport-requested with a message indicating that it had to land with this one.

I'm definitely open to other ideas. The real issue is that we don't have a documented and easy way to express relations between pull requests.

addaleax · 2020-04-28T18:43:41Z

@targos Yeah, I’d be a fan of having something more specific than Refs:, e.g. Must-Be-Released-With: <pr-url> that would indicate that PRs should have to land together… I’ll put this on my list of things that I can try to implement

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 8, 2020

addaleax added the worker Issues and PRs related to Worker support. label Feb 8, 2020

santigimeno approved these changes Feb 8, 2020

View reviewed changes

devnexen approved these changes Feb 8, 2020

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 8, 2020

cjihrig approved these changes Feb 9, 2020

View reviewed changes

jasnell approved these changes Feb 10, 2020

View reviewed changes

addaleax closed this Feb 10, 2020

addaleax deleted the fix-messageport-dtor branch February 10, 2020 16:29

codebytere mentioned this pull request Feb 17, 2020

v13.9.0 proposal #31837

Merged

codebytere added the backport-requested-v12.x label Mar 15, 2020

targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v12.x labels Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

src: do not unnecessarily re-assign uv handle data#31696

src: do not unnecessarily re-assign uv handle data#31696
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:fix-messageport-dtor

addaleax commented Feb 8, 2020 •

edited

Loading

Uh oh!

nodejs-github-bot commented Feb 8, 2020

Uh oh!

nodejs-github-bot commented Feb 8, 2020 •

edited by addaleax

Loading

Uh oh!

addaleax commented Feb 10, 2020

Uh oh!

codebytere commented Mar 15, 2020

Uh oh!

addaleax commented Apr 28, 2020

Uh oh!

targos commented Apr 28, 2020 •

edited

Loading

Uh oh!

addaleax commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

addaleax commented Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Feb 8, 2020

Uh oh!

nodejs-github-bot commented Feb 8, 2020 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Feb 10, 2020

Uh oh!

codebytere commented Mar 15, 2020

Uh oh!

addaleax commented Apr 28, 2020

Uh oh!

targos commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

addaleax commented Feb 8, 2020 •

edited

Loading

nodejs-github-bot commented Feb 8, 2020 •

edited by addaleax

Loading

targos commented Apr 28, 2020 •

edited

Loading