From 5d73ce3748886087e04b667efe5646beb075c331 Mon Sep 17 00:00:00 2001
From: Mooneer Salem <mooneer@gmail.com>
Date: Tue, 15 Sep 2026 22:29:23 -0700
Subject: [PATCH] Harden reliableTextFifo_ read and fix playbackResampler_ data
 race

FreeDVInterface::getReliableText() ignored read()'s return value into an
uninitialized stack buffer; a failed read (currently unreachable given the
FIFO's one-record capacity and mutex-serialized consumer side, but fragile)
would have built a std::string from uninitialized stack memory. Now
zero-inits the buffer and only uses it on success.

PlaybackStep::playbackResampler_ was a raw pointer whose lifecycle is owned
exclusively by the file I/O thread (nonRtThreadEntry_), but reset() -- called
on the real-time pipeline thread on every RX->TX transition -- read and
dereferenced it without any synchronization. That's a torn-pointer read, a
possible use-after-free (racing the file I/O thread's delete/recreate on a
sample-rate change), and a possible concurrent mutation of the underlying
libsamplerate state during a live execute(). Since the RT thread can't take
a lock, reset() now just raises a lock-free atomic flag; the file I/O thread
consumes it and performs the actual resampler reset itself.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018JYqaVNT7DbgmKePHQtNYv
---
 src/freedv_interface.cpp      |  8 +++++---
 src/pipeline/PlaybackStep.cpp | 17 +++++++++++++----
 src/pipeline/PlaybackStep.h   |  7 +++++++
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/src/freedv_interface.cpp b/src/freedv_interface.cpp
index c1329d8b..5838fa5c 100644
--- a/src/freedv_interface.cpp
+++ b/src/freedv_interface.cpp
@@ -96,9 +96,11 @@ const char* FreeDVInterface::getReliableText()
     
     if (reliableTextFifo_.numUsed() > 0)
     {
-        char tmpBuf[RELIABLE_TEXT_FIFO_SIZE];
-        reliableTextFifo_.read(tmpBuf, RELIABLE_TEXT_FIFO_SIZE);
-        receivedReliableText_ = tmpBuf;
+        char tmpBuf[RELIABLE_TEXT_FIFO_SIZE] = { 0 };
+        if (reliableTextFifo_.read(tmpBuf, RELIABLE_TEXT_FIFO_SIZE) == 0)
+        {
+            receivedReliableText_ = tmpBuf;
+        }
     }
     
     char* ret = new char[receivedReliableText_.size() + 1];
diff --git a/src/pipeline/PlaybackStep.cpp b/src/pipeline/PlaybackStep.cpp
index b3917882..031ebbd8 100644
--- a/src/pipeline/PlaybackStep.cpp
+++ b/src/pipeline/PlaybackStep.cpp
@@ -63,6 +63,7 @@ PlaybackStep::PlaybackStep(
     , nonRtThreadEnding_(false)
     , playbackResampler_(nullptr)
     , outputFifo_(inputSampleRate * NUM_SECONDS_TO_READ)
+    , resamplerResetRequested_(false)
 {
     // Pre-allocate buffers so we don't have to do so during real-time operation.
     outputSamples_ = std::make_unique<short[]>(inputSampleRate_);
@@ -124,6 +125,13 @@ void PlaybackStep::nonRtThreadEntry_()
     while (!nonRtThreadEnding_.load(std::memory_order_acquire))
     {
         g_mutexProtectingCallbackData.Lock();
+
+        if (resamplerResetRequested_.exchange(false, std::memory_order_acquire) &&
+            playbackResampler_ != nullptr)
+        {
+            playbackResampler_->reset();
+        }
+
         auto playFile = getSndFileFn_();
         if (playFile != nullptr)
         {
@@ -224,10 +232,11 @@ void PlaybackStep::nonRtThreadEntry_()
 
 void PlaybackStep::reset() FREEDV_NONBLOCKING
 {
-    if (playbackResampler_ != nullptr)
-    {
-        playbackResampler_->reset();
-    }
+    // Don't touch playbackResampler_ directly here -- it's owned by
+    // nonRtThreadEntry_() on the file I/O thread, which may be concurrently
+    // recreating or executing it. This thread is real-time and must not
+    // lock, so just flag the request; the owning thread will apply it.
+    resamplerResetRequested_.store(true, std::memory_order_release);
 
     outputFifo_.reset();
 
diff --git a/src/pipeline/PlaybackStep.h b/src/pipeline/PlaybackStep.h
index 84016829..dde0898a 100644
--- a/src/pipeline/PlaybackStep.h
+++ b/src/pipeline/PlaybackStep.h
@@ -70,6 +70,13 @@ private:
     ResampleStep* playbackResampler_;
     GenericFIFO<short> outputFifo_;
 
+    // playbackResampler_ is created/destroyed/executed only by nonRtThreadEntry_()
+    // (the file I/O thread). reset() runs on the real-time pipeline thread, which
+    // must not block on a lock or touch a pointer that thread doesn't own -- so it
+    // just raises this flag, and nonRtThreadEntry_() performs the actual
+    // playbackResampler_->reset() on its own thread the next time it wakes up.
+    std::atomic<bool> resamplerResetRequested_;
+
     void nonRtThreadEntry_();
 };