Skip to content

Commit

Permalink
async_hooks: avoid decrementing iterator after erase
Browse files Browse the repository at this point in the history
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: nodejs/node#42749
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
  • Loading branch information
dygabo authored and guangwong committed Oct 10, 2022
1 parent 7334bb5 commit 5690fe8
Showing 1 changed file with 5 additions and 7 deletions.
12 changes: 5 additions & 7 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local<v8::Function> 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)
Expand Down Expand Up @@ -251,12 +250,11 @@ inline void AsyncHooks::AddContext(v8::Local<v8::Context> ctx) {
inline void AsyncHooks::RemoveContext(v8::Local<v8::Context> 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<v8::Context> saved_context =
PersistentToLocal::Weak(isolate, *it);
if (saved_context == ctx) {
Expand Down

0 comments on commit 5690fe8

Please sign in to comment.