[Lldb-commits] [lldb] [lldb] Don't unregister a listener that's being destroyed (PR #97300)

[Pavel Labath via lldb-commits]

https://github.com/labath created https://github.com/llvm/llvm-project/pull/97300

It's not necessary because the broadcasters (and broadcast managers) hold a weak_ptr (*) to it, and will delete the weak_ptr next time they try to lock it. Doing this prevents recursion in RemoveListener, where the function can end up holding the only shared_ptr to a listener, and its destruction can trigger another call to RemoveListener -- which will mess up the state of the first instance.

This is the same bug that we've have fixed in
https://reviews.llvm.org/D23406, but it was effectively undone in https://reviews.llvm.org/D157556. With the addition of a primary listener, a fix like D23406 becomes unwieldy (and it has already shown itself to be fragile), which is why this patch attempts a different approach.

Like in 2016, I don't know a good way to unit test this bug, since it depends on precise timing, but the thing I like about this approach is that it enables us to change the broadcaster mutex into a non-recursive one. While that doesn't prevent the bug from happening again, it will make it much easier to spot in the future, as the code will hang with a smoking gun (instead of crashing a little while later). I'm going to attempt that in a separate patch to minimize disruption.

(*) Technically a broadcaster holds the *primary* listener as a shared_ptr. That's probably not a good idea, because it means the listener will never get destroyed (unless it is explicitly unregistered), but it also means that this change has no impact on primary listeners.

>From 542d54217a060594e3bd51782194d3bbfbbff7e2 Mon Sep 17 00:00:00 2001
From: Pavel Labath <pavel at labath.sk>
Date: Mon, 1 Jul 2024 10:48:51 +0000
Subject: [PATCH] [lldb] Don't unregister a listener that's being destroyed

It's not necessary because the broadcasters (and broadcast managers)
hold a weak_ptr (*) to it, and will delete the weak_ptr next time they try
to lock it. Doing this prevents recursion in RemoveListener, where the
function can end up holding the only shared_ptr to a listener, and its
destruction can trigger another call to RemoveListener -- which will
mess up the state of the first instance.

This is the same bug that we've have fixed in
https://reviews.llvm.org/D23406, but it was effectively undone in
https://reviews.llvm.org/D157556. With the addition of a primary
listener, a fix like D23406 becomes unwieldy (and it has already shown
itself to be fragile), which is why this patch attempts a different
approach.

Like in 2016, I don't know a good way to unit test this bug, since it
depends on precise timing, but the thing I like about this approach is
that it enables us to change the broadcaster mutex into a non-recursive
one. While that doesn't prevent the bug from happening again, it will
make it much easier to spot in the future, as the code will hang with a
smoking gun (instead of crashing a little while later). I'm going to
attempt that in a separate patch to minimize disruption.

(*) Technically a broadcaster holds the *primary* listener as a
shared_ptr. That's probably not a good idea, because it means the
listener will never get destroyed (unless it is explicitly
unregistered), but it also means that this change has no impact on
primary listeners.
---
 lldb/source/Utility/Listener.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lldb/source/Utility/Listener.cpp b/lldb/source/Utility/Listener.cpp
index 6a74c530ad257..5aacb4104e1cf 100644
--- a/lldb/source/Utility/Listener.cpp
+++ b/lldb/source/Utility/Listener.cpp
@@ -30,7 +30,7 @@ Listener::Listener(const char *name)
 Listener::~Listener() {
   Log *log = GetLog(LLDBLog::Object);
 
-  Clear();
+  // Don't call Clear() from here as that can cause races. See #96750.
 
   LLDB_LOGF(log, "%p Listener::%s('%s')", static_cast<void *>(this),
             __FUNCTION__, m_name.c_str());