From 5690fe85a3f8908b722ef0bf38e115d49d841306 Mon Sep 17 00:00:00 2001 From: Gabriel Bota <94833492+dygabo@users.noreply.github.com> Date: Sun, 17 Apr 2022 16:37:14 +0200 Subject: [PATCH] async_hooks: avoid decrementing iterator after erase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit decrementing an iterator returned by `std::vector::erase` may have undefined behaviour and should not be used. Decrementing `end()` on an empty container is undefined and `.erase()` could leave the container empty. Instead, by calling `vec.erase(it--)` we decrement the valid iterator before the erase operation but after being passed to the erase method. In case of `AsyncHooks::RemoveContext` perform the cleanup of empty contexts upfront using `std::remove_if` because the iteration gets interrupted as soon as the context to be removed has been found. PR-URL: https://github.com/nodejs/node/pull/42749 Reviewed-By: Anna Henningsen Reviewed-By: Rich Trott Reviewed-By: Mohammed Keyvanzadeh Reviewed-By: Tobias Nießen Reviewed-By: James M Snell Reviewed-By: Darshan Sen --- src/env-inl.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 4a34393cad7..e0093ebcfec 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -107,8 +107,7 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local init, js_promise_hooks_[3].Reset(env()->isolate(), resolve); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { if (it->IsEmpty()) { - it = contexts_.erase(it); - it--; + contexts_.erase(it--); continue; } PersistentToLocal::Weak(env()->isolate(), *it) @@ -251,12 +250,11 @@ inline void AsyncHooks::AddContext(v8::Local ctx) { inline void AsyncHooks::RemoveContext(v8::Local ctx) { v8::Isolate* isolate = env()->isolate(); v8::HandleScope handle_scope(isolate); + contexts_.erase(std::remove_if(contexts_.begin(), + contexts_.end(), + [&](auto&& el) { return el.IsEmpty(); }), + contexts_.end()); for (auto it = contexts_.begin(); it != contexts_.end(); it++) { - if (it->IsEmpty()) { - it = contexts_.erase(it); - it--; - continue; - } v8::Local saved_context = PersistentToLocal::Weak(isolate, *it); if (saved_context == ctx) {