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_018JYqaVNT7DbgmKePHQtNYvms-rade-v2
parent
ce42017a15
commit
5d73ce3748
|
|
@ -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];
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
||||
|
|
|
|||
|
|
@ -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_();
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue