From 1835a143d62fd9095c76a6526edaae5e578d8441 Mon Sep 17 00:00:00 2001
From: Ion Reguera <ion@stagelab.coop>
Date: Tue, 12 May 2026 22:07:41 +0200
Subject: [PATCH] rtppeer: hold self-shared_ptr across send_goodbye to fix UAF
 in disconnect()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

rtppeer_t::disconnect() called send_goodbye() and then reset() on *this.
send_goodbye() synchronously fires status_change_event, whose
rtpserverpeer slot calls rtpserver_t::remove_peer(). remove_peer() does
peers.erase() on the owning vector, destructing the rtpserverpeer_t and
dropping the last shared_ptr<rtppeer_t> — freeing *this. Control then
returned up the stack to reset(), reading and writing freed memory.

ASAN confirmed (heap-use-after-free, WRITE of size 4 at rtppeer.cpp:55,
free chain disconnect -> send_goodbye -> status_change_event ->
rtpserverpeer_t::status_change -> rtpserver_t::remove_peer ->
vector::erase -> ~rtpserverpeer_t -> ~shared_ptr<rtppeer_t> -> freed).

Fix: take a local shared_ptr at entry via weak_from_this().lock(). When
the rtppeer is shared-owned (the server path that originally crashed),
the local keeps it alive across the signal storm. When it isn't
(rtpclient_t embeds rtppeer_t by value at rtpclient.hpp:45; tests
stack-allocate it in tests/test_rtppeer.cpp), no external owner can drop
the last reference mid-call, so the UAF is structurally impossible —
and weak_from_this().lock() yielding nullptr is harmless.

In production this crashed ~317 times per boot under steady cluster
activity (~15-20s interval). It also explains the libfmt-vformat crash
signature previously reported on v24.12 — same root-cause UAF, the
libfmt allocator just happened to be the next user of the freed slab.
---
 include/rtpmidid/rtppeer.hpp |  3 ++-
 lib/rtppeer.cpp              | 11 +++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/rtpmidid/rtppeer.hpp b/include/rtpmidid/rtppeer.hpp
index 86741dd6..e781405d 100644
--- a/include/rtpmidid/rtppeer.hpp
+++ b/include/rtpmidid/rtppeer.hpp
@@ -23,6 +23,7 @@
 #include "signal.hpp"
 #include "stats.hpp"
 #include <arpa/inet.h>
+#include <memory>
 #include <string>
 
 namespace rtpmidid {
@@ -41,7 +42,7 @@ class bad_midi_packet : public ::rtpmidid::exception {
       : ::rtpmidid::exception("Bad MIDI packet: {}", what) {}
 };
 
-class rtppeer_t {
+class rtppeer_t : public std::enable_shared_from_this<rtppeer_t> {
   NON_COPYABLE_NOR_MOVABLE(rtppeer_t)
 public:
   // Commands, the id is the same chars as the name
diff --git a/lib/rtppeer.cpp b/lib/rtppeer.cpp
index 56cf80d4..a72b1f4f 100644
--- a/lib/rtppeer.cpp
+++ b/lib/rtppeer.cpp
@@ -858,6 +858,17 @@ void rtppeer_t::parse_journal_chapter_N(uint8_t channel,
   }
 }
 void rtppeer_t::disconnect() {
+  // Keep ourselves alive across send_goodbye(): it fires status_change_event,
+  // whose rtpserverpeer slot calls rtpserver_t::remove_peer(), which erases
+  // the owning rtpserverpeer_t from a vector and drops the last
+  // shared_ptr<rtppeer_t>. Without this local, the reset() below would
+  // run on freed memory.
+  //
+  // weak_from_this().lock() returns nullptr when this instance isn't owned
+  // by a shared_ptr (rtpclient_t embeds rtppeer_t by value; tests construct
+  // it on the stack). In that case no external owner exists to drop us
+  // mid-call, so the UAF can't occur and proceeding without self is safe.
+  auto self = weak_from_this().lock();
   if (status & MIDI_CONNECTED) {
     send_goodbye(MIDI_PORT);
   }