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

async_wrap: close the destroy_ids_idle_handle_ on environment destruction #10385

Closed
wants to merge 3 commits into
base: master
from

Conversation

@reshnm
Contributor

reshnm commented Dec 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

async_wrap

Description of change

The destroy_ids_idle_handle_ is currently not getting closed in the environment destructor. After the environment has been destroyed, the uv loop used by the environment contains a dangling pointer in the handle list which leads to undefined behavior when the loop is still being used.

This is currently no issue for the "main" uv loop used by node. However if an embedder creates his own environment this will most likely lead to crashes. By looking at the code in Environment::Start it was also not entirely clear for me that destroy_ids_idle_handle_ is not added to the handle cleanup queue by intention.

The additional delayed cleanup queue may be an overkill since it only contains one handle right now, but it seemed to me that it would be the most generic and verbose approach.

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on environment destruction.
Not closing the handle leaves a dangling pointer in the used uv loop.
This leads to undefined behavior when the uv loop is used after
the environment has been destroyed.

@reshnm reshnm changed the title from [async_wrap] close the destroy_ids_idle_handle_ on environment destruction to async_wrap: close the destroy_ids_idle_handle_ on environment destruction Dec 21, 2016

@addaleax addaleax requested review from bnoordhuis and trevnorris Dec 21, 2016

@addaleax

Seems okay but I agree that this approach looks a bit like overkill, let’s hear what others think.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Dec 22, 2016

Member

A rule of thumb I use is "don't generalize until there are at least two or three instances." In this particular case, I'd just close the idle handle last in the destructor.

Aside: am I to infer SAP uses node in their products? It sure is a step up from ABAP. :-)

(I spent most of 2004 writing ABAP. I don't hate it, actually.)

Member

bnoordhuis commented Dec 22, 2016

A rule of thumb I use is "don't generalize until there are at least two or three instances." In this particular case, I'd just close the idle handle last in the destructor.

Aside: am I to infer SAP uses node in their products? It sure is a step up from ABAP. :-)

(I spent most of 2004 writing ABAP. I don't hate it, actually.)

async wrap: removed the delayed handle cleanup queue
Do not add the destroy_ids_idle_handle_ to a separate handle clenup queue.
Call uv_close on the handle directly in the environment destructor.
@reshnm

This comment has been minimized.

Show comment
Hide comment
@reshnm

reshnm Dec 22, 2016

Contributor

Good point. I moved the cleanup code completely to the environment destructor.

@bnoordhuis You are right, we use node in our products. Your ABAP programming skills are probably better than mine. I have never written a single line of code in ABAP :-)

Contributor

reshnm commented Dec 22, 2016

Good point. I moved the cleanup code completely to the environment destructor.

@bnoordhuis You are right, we use node in our products. Your ABAP programming skills are probably better than mine. I have never written a single line of code in ABAP :-)

async_wrap: initialize member handle_cleanup_waiting_
Member handle_cleanup_waiting_ must be set to one before
the uv_close on destroy_ids_idle_handle_ is called.

jasnell added a commit that referenced this pull request Dec 24, 2016

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: #10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Dec 24, 2016

Member

Landed in c7ff96b

Member

jasnell commented Dec 24, 2016

Landed in c7ff96b

@jasnell jasnell closed this Dec 24, 2016

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: nodejs#10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 19, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: nodejs#10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 24, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: nodejs#10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Jan 27, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: nodejs#10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

@italoacasas italoacasas referenced this pull request Jan 29, 2017

Merged

v7.5.0 proposal #11062

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Mar 7, 2017

Member

@trevnorris should this be backported? Do you want to do a batch update to async_wrap across the LTS lines?

Member

MylesBorins commented Mar 7, 2017

@trevnorris should this be backported? Do you want to do a batch update to async_wrap across the LTS lines?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins
Member

MylesBorins commented Apr 13, 2017

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins May 8, 2017

Member

ping

Member

MylesBorins commented May 8, 2017

ping

addaleax added a commit to addaleax/node that referenced this pull request May 9, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: nodejs#10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax May 9, 2017

Member

backport in #12922

Member

addaleax commented May 9, 2017

backport in #12922

MylesBorins added a commit that referenced this pull request May 16, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: #10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request May 18, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: #10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request May 23, 2017

Merged

v6.11.0 proposal #13059

andrew749 added a commit to michielbaird/node that referenced this pull request Jul 19, 2017

async_wrap: close the destroy_ids_idle_handle_
The destroy_ids_idle_handle_ needs to be closed on
environment destruction. Not closing the handle leaves
a dangling pointer in the used uv loop. This leads to
undefined behavior when the uv loop is used after the
environment has been destroyed.

PR-URL: nodejs/node#10385
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment