From 38de40ac50516d49aaf4ad62e4a0e18fea50d58e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 8 Feb 2020 16:57:02 +0100 Subject: [PATCH] src: do not unnecessarily re-assign uv handle data MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a555be2e45b283 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: https://github.com/nodejs/node/pull/31605 PR-URL: https://github.com/nodejs/node/pull/31696 Reviewed-By: Santiago Gimeno Reviewed-By: David Carlier Reviewed-By: Colin Ihrig Reviewed-By: James M Snell --- src/handle_wrap.cc | 1 + src/node_messaging.cc | 9 ++++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/handle_wrap.cc b/src/handle_wrap.cc index f5d622fc255cdf..4e039d5dbf2d37 100644 --- a/src/handle_wrap.cc +++ b/src/handle_wrap.cc @@ -115,6 +115,7 @@ HandleWrap::HandleWrap(Environment* env, void HandleWrap::OnClose(uv_handle_t* handle) { + CHECK_NOT_NULL(handle->data); BaseObjectPtr wrap { static_cast(handle->data) }; wrap->Detach(); diff --git a/src/node_messaging.cc b/src/node_messaging.cc index c7c46063731a5b..2de1c824d206d2 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -515,10 +515,9 @@ MessagePort::MessagePort(Environment* env, CHECK_EQ(uv_async_init(env->event_loop(), &async_, onmessage), 0); - async_.data = nullptr; // Reset later to indicate success of the constructor. - auto cleanup = OnScopeLeave([&]() { - if (async_.data == nullptr) Close(); - }); + // Reset later to indicate success of the constructor. + bool succeeded = false; + auto cleanup = OnScopeLeave([&]() { if (!succeeded) Close(); }); Local fn; if (!wrap->Get(context, env->oninit_symbol()).ToLocal(&fn)) @@ -535,7 +534,7 @@ MessagePort::MessagePort(Environment* env, return; emit_message_fn_.Reset(env->isolate(), emit_message_fn); - async_.data = static_cast(this); + succeeded = true; Debug(this, "Created message port"); }