journal/ObjectPlayer: don't acquire locks in destructor

~ObjectPlayer took m_timer_lock only to assert two invariants. but that
lock is borrowed by reference from the caller's SafeTimer, and an
ObjectPlayer can outlive it: a C_Fetch/C_WatchFetch completion on the
librados finisher may hold the last reference and run ~ObjectPlayer after
the timer and its lock are already gone. re-taking the freed lock is a
heap-use-after-free, which unittest_journal hits on arm64 under ASan:

  ==ERROR: AddressSanitizer: heap-use-after-free
    journal::ObjectPlayer::~ObjectPlayer  ObjectPlayer.cc:63
    journal::ObjectPlayer::C_WatchFetch::~C_WatchFetch
    journal::ObjectPlayer::C_Fetch::finish

the lock isn't needed though: at refcount 0 the watch has been cancelled
(m_watch_ctx == nullptr, asserted below) so no timer task references us, and
no fetch is in flight since a pending fetch holds a reference. nothing else
can touch our state. Furthermore, we can also skip acquiring `m_lock` as
well, because, in the destructor, it shouldn't really matter -- if one
of these asserts fails because the execution of the destructor races
with some `ObjectPlayer` mthod, we would get what the `assert()` was
added for. They are here to catch bugs and such a race just being
possible is a bug in itself.

Fixes: https://tracker.ceph.com/issues/77496
Signed-off-by: Kefu Chai <k.chai@proxmox.com>

diff --git a/src/journal/ObjectPlayer.cc b/src/journal/ObjectPlayer.cc
index 29893cec2d91c1ea204b1e3dba14dd585fabd6c9..e01355e2f72cf305928744d8f32d28ba68da9aca 100644 (file)
--- a/src/journal/ObjectPlayer.cc
+++ b/src/journal/ObjectPlayer.cc
@@ -59,12 +59,8 @@ ObjectPlayer::ObjectPlayer(librados::IoCtx &ioctx,
 }
 
 ObjectPlayer::~ObjectPlayer() {
-  {
-    std::lock_guard timer_locker{m_timer_lock};
-    std::lock_guard locker{m_lock};
-    ceph_assert(!m_fetch_in_progress);
-    ceph_assert(m_watch_ctx == nullptr);
-  }
+  ceph_assert(!m_fetch_in_progress);
+  ceph_assert(m_watch_ctx == nullptr);
 }
 
 void ObjectPlayer::fetch(Context *on_finish) {