Skip to content

Commit 5cf34f7

Browse files
everett1992sreehariannamnsavoire
committed
test: add regression test for cleanup hook self-removal UAF
Add a cctest that registers an environment cleanup hook which removes itself while the cleanup queue is drained -- the ordinary teardown path for every node::ObjectWrap still alive at exit since #63642. It exercises CleanupHookThunkRun(), which must not read the CleanupHookThunk after invoking the hook, because the hook has already erased and freed it. The use-after-free is silent in ordinary builds and is caught by the ASan/Valgrind CI, which is how the original assertion (#63923) surfaced. Refs: #65195 Co-authored-by: Sreehari Annam <sreehari.annam@gmail.com> Co-authored-by: nsavoire <19255994+nsavoire@users.noreply.github.com> Signed-off-by: Caleb Everett <everett.caleb@gmail.com>
1 parent 893ed58 commit 5cf34f7

1 file changed

Lines changed: 39 additions & 0 deletions

File tree

‎test/cctest/test_environment.cc‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,12 @@ static void at_exit_callback_ordered2(void* arg);
2828
static void at_exit_js(void* arg);
2929
static std::string cb_1_arg; // NOLINT(runtime/string)
3030

31+
struct SelfRemovingCleanupHookState {
32+
v8::Isolate* isolate;
33+
bool ran = false;
34+
};
35+
static void self_removing_cleanup_hook(void* arg);
36+
3137
class EnvironmentTest : public EnvironmentTestFixture {
3238
private:
3339
void TearDown() override {
@@ -289,6 +295,26 @@ TEST_F(EnvironmentTest, AtExitRunsJS) {
289295
EXPECT_TRUE(called_at_exit_js);
290296
}
291297

298+
// A cleanup hook that removes itself while the environment cleanup queue is
299+
// being drained must not cause a use-after-free. Since #63642 this is the
300+
// ordinary teardown path for every node::ObjectWrap still alive at exit.
301+
// The use-after-free is silent in ordinary builds; it is caught by the
302+
// ASan/Valgrind CI, which is also how the original assertion (#63923)
303+
// surfaced. Regression test for https://github.com/nodejs/node/issues/65195.
304+
TEST_F(EnvironmentTest, RemoveEnvironmentCleanupHookDuringCleanup) {
305+
const v8::HandleScope handle_scope(isolate_);
306+
const Argv argv;
307+
SelfRemovingCleanupHookState state{isolate_};
308+
{
309+
Env env{handle_scope, argv};
310+
node::AddEnvironmentCleanupHook(
311+
isolate_, self_removing_cleanup_hook, &state);
312+
// Destroying `env` runs FreeEnvironment() -> RunCleanup(), which drains
313+
// the cleanup queue and invokes CleanupHookThunkRun() for the hook above.
314+
}
315+
EXPECT_TRUE(state.ran);
316+
}
317+
292318
TEST_F(EnvironmentTest, MultipleEnvironmentsPerIsolate) {
293319
const v8::HandleScope handle_scope(isolate_);
294320
const Argv argv;
@@ -372,6 +398,19 @@ static void at_exit_js(void* arg) {
372398
called_at_exit_js = true;
373399
}
374400

401+
// Mirrors what node::ObjectWrap does since
402+
// https://github.com/nodejs/node/pull/63642: the destructor removes the
403+
// object's own environment cleanup hook. When that runs while the cleanup
404+
// queue is being drained, CleanupHookThunkRun() must not read the
405+
// CleanupHookThunk after invoking the hook -- the hook has already erased and
406+
// freed it. See https://github.com/nodejs/node/issues/65195.
407+
static void self_removing_cleanup_hook(void* arg) {
408+
auto* state = static_cast<SelfRemovingCleanupHookState*>(arg);
409+
state->ran = true;
410+
node::RemoveEnvironmentCleanupHook(
411+
state->isolate, self_removing_cleanup_hook, state);
412+
}
413+
375414
TEST_F(EnvironmentTest, SetImmediateCleanup) {
376415
int called = 0;
377416
int called_unref = 0;

0 commit comments

Comments
 (0)