vdr February 2023

vdr@linuxtv.org

4 participants
4 discussions

[PATCH] Add thread safety to cRingBufferLinear
by Patrick Lerda 25 Feb '23

25 Feb '23

Beside preventing crashes with vdr-2.6.3 this is required to get vdr to work properly with the gcc thread sanitizer. --- ringbuffer.c | 18 +++++++++++++++++- ringbuffer.h | 3 +++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/ringbuffer.c b/ringbuffer.c index 902c887..1c24df2 100644 --- a/ringbuffer.c +++ b/ringbuffer.c @@ -210,12 +210,16 @@ int cRingBufferLinear::DataReady(const uchar *Data, int Count) int cRingBufferLinear::Available(void) { + Lock(); int diff = … [View More]head - tail; - return (diff >= 0) ? diff : Size() + diff - margin; + int available = (diff >= 0) ? diff : Size() + diff - margin; + Unlock(); + return available; } void cRingBufferLinear::Clear(void) { + Lock(); int Head = head; tail = Head; #ifdef DEBUGRINGBUFFERS @@ -224,11 +228,13 @@ void cRingBufferLinear::Clear(void) lastPut = lastGet = -1; #endif maxFill = 0; + Unlock(); EnablePut(); } int cRingBufferLinear::Read(int FileHandle, int Max) { + Lock(); int Tail = tail; int diff = Tail - head; int free = (diff > 0) ? diff - 1 : Size() - head; @@ -259,6 +265,7 @@ int cRingBufferLinear::Read(int FileHandle, int Max) lastHead = head; lastPut = Count; #endif + Unlock(); EnableGet(); if (free == 0) WaitForPut(); @@ -267,6 +274,7 @@ int cRingBufferLinear::Read(int FileHandle, int Max) int cRingBufferLinear::Read(cUnbufferedFile *File, int Max) { + Lock(); int Tail = tail; int diff = Tail - head; int free = (diff > 0) ? diff - 1 : Size() - head; @@ -297,6 +305,7 @@ int cRingBufferLinear::Read(cUnbufferedFile *File, int Max) lastHead = head; lastPut = Count; #endif + Unlock(); EnableGet(); if (free == 0) WaitForPut(); @@ -306,6 +315,7 @@ int cRingBufferLinear::Read(cUnbufferedFile *File, int Max) int cRingBufferLinear::Put(const uchar *Data, int Count) { if (Count > 0) { + Lock(); int Tail = tail; int rest = Size() - head; int diff = Tail - head; @@ -336,6 +346,7 @@ int cRingBufferLinear::Put(const uchar *Data, int Count) lastHead = head; lastPut = Count; #endif + Unlock(); EnableGet(); if (Count == 0) WaitForPut(); @@ -345,6 +356,7 @@ int cRingBufferLinear::Put(const uchar *Data, int Count) uchar *cRingBufferLinear::Get(int &Count) { + Lock(); int Head = head; if (getThreadTid <= 0) getThreadTid = cThread::ThreadId(); @@ -362,14 +374,17 @@ uchar *cRingBufferLinear::Get(int &Count) uchar *p = buffer + tail; if ((cont = DataReady(p, cont)) > 0) { Count = gotten = cont; + Unlock(); return p; } + Unlock(); WaitForGet(); return NULL; } void cRingBufferLinear::Del(int Count) { + Lock(); if (Count > gotten) { esyslog("ERROR: invalid Count in cRingBufferLinear::Del: %d (limited to %d)", Count, gotten); Count = gotten; @@ -387,6 +402,7 @@ void cRingBufferLinear::Del(int Count) lastTail = tail; lastGet = Count; #endif + Unlock(); } // --- cFrame ---------------------------------------------------------------- diff --git a/ringbuffer.h b/ringbuffer.h index 746fc51..cbaa12c 100644 --- a/ringbuffer.h +++ b/ringbuffer.h @@ -58,10 +58,13 @@ public: static void PrintDebugRBL(void); #endif private: + cMutex mutex; int margin, head, tail; int gotten; uchar *buffer; char *description; + void Lock(void) { mutex.Lock(); } + void Unlock(void) { mutex.Unlock(); } protected: virtual int DataReady(const uchar *Data, int Count); ///< By default a ring buffer has data ready as soon as there are at least -- 2.39.1 [View Less]

5 12

[PATCH] Fix cThread related race conditions
by Patrick Lerda 19 Feb '23

19 Feb '23

This change fixes the following issue: ==15457==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002d0 (pc 0x7fd2f4301710 bp 0x7fd2b5552a30 sp 0x7fd2b5552988 T228) ==15457==The signal is caused by a READ memory access. ==15457==Hint: address points to the zero page. #0 0x7fd2f4301710 in pthread_detach (/lib64/libc.so.6+0x8b710) #1 0xdb4430 in cThread::Start() vdr-2.6.3/thread.c:317 #2 0x7fd2e5ea8fc4 in cIptvStreamer::Open() vdr-2.6.3/PLUGINS/src/iptv/streamer.c:66 #3 … [View More]0x7fd2e5e1e8c2 in cIptvDevice::OpenDvr() vdr-2.6.3/PLUGINS/src/iptv/device.c:342 #4 0x634903 in cDevice::Action() vdr-2.6.3/device.c:1714 #5 0xdb644a in cThread::StartThread(cThread*) vdr-2.6.3/thread.c:293 #6 0x7fd2f4300729 (/lib64/libc.so.6+0x8a729) #7 0x7fd2f437d0bb (/lib64/libc.so.6+0x1070bb) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV (/lib64/libc.so.6+0x8b710) in pthread_detach Thread T228 (device 5 receiv) created by T222 (cLiveStreamer s) here: #0 0x7fd2f52d4716 in pthread_create (/usr/lib64/libasan.so.6+0x59716) #1 0xdb437b in cThread::Start() vdr-2.6.3/thread.c:316 #2 0x63a3ad in cDevice::AttachReceiver(cReceiver*) vdr-2.6.3/device.c:1844 #3 0x7fd2e8162c56 in cDummyReceiver::Create(cDevice*) vdr-2.6.3/PLUGINS/src/vnsiserver/videoinput.c:472 #4 0x7fd2e816dbc2 in cVideoInput::Open(cChannel const*, int, cVideoBuffer*) vdr-2.6.3/PLUGINS/src/vnsiserver/videoinput.c:530 #5 0x7fd2e80d78d3 in cLiveStreamer::Action() vdr-2.6.3/PLUGINS/src/vnsiserver/streamer.c:318 #6 0xdb644a in cThread::StartThread(cThread*) vdr-2.6.3/thread.c:293 #7 0x7fd2f4300729 (/lib64/libc.so.6+0x8a729) --- thread.c | 25 +++++++++++++++---------- thread.h | 11 +++++++---- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/thread.c b/thread.c index 93eb8c0..21be7a4 100644 --- a/thread.c +++ b/thread.c @@ -312,13 +312,16 @@ bool cThread::Start(void) cCondWait::SleepMs(THREAD_STOP_SLEEP); } if (!active) { - active = running = true; - if (pthread_create(&childTid, NULL, (void *(*) (void *))&StartThread, (void *)this) == 0) { - pthread_detach(childTid); // auto-reap + pthread_t localTid; + running = true; + if (pthread_create(&localTid, NULL, (void *(*) (void *))&StartThread, (void *)this) == 0) { + pthread_detach(localTid); // auto-reap + childTid = localTid; + active = true; } else { LOG_ERROR; - active = running = false; + running = false; return false; } } @@ -339,11 +342,12 @@ bool cThread::Active(void) // performed but no signal is actually sent. // int err; - if ((err = pthread_kill(childTid, 0)) != 0) { + const pthread_t localTid = childTid; + if ((err = pthread_kill(localTid, 0)) != 0) { if (err != ESRCH) LOG_ERROR; - childTid = 0; - active = running = false; + if (active && childTid == localTid) + active = running = false; } else return true; @@ -355,6 +359,7 @@ void cThread::Cancel(int WaitSeconds) { running = false; if (active && WaitSeconds > -1) { + const pthread_t localTid = childTid; if (WaitSeconds > 0) { for (time_t t0 = time(NULL) + WaitSeconds; time(NULL) < t0; ) { if (!Active()) @@ -363,9 +368,9 @@ void cThread::Cancel(int WaitSeconds) } esyslog("ERROR: %s thread %d won't end (waited %d seconds) - canceling it...", description ? description : "", childThreadId, WaitSeconds); } - pthread_cancel(childTid); - childTid = 0; - active = false; + pthread_cancel(localTid); + if (active && childTid == localTid) + active = false; } } diff --git a/thread.h b/thread.h index 16c4bd7..06046ea 100644 --- a/thread.h +++ b/thread.h @@ -13,6 +13,7 @@ #include <pthread.h> #include <stdio.h> #include <sys/types.h> +#include <atomic> typedef pid_t tThreadId; @@ -56,7 +57,7 @@ class cRwLock { private: pthread_rwlock_t rwlock; int locked; - tThreadId writeLockThreadId; + std::atomic<tThreadId> writeLockThreadId; public: cRwLock(bool PreferWriter = false); ~cRwLock(); @@ -79,9 +80,11 @@ public: class cThread { friend class cThreadLock; private: - bool active; - bool running; - pthread_t childTid; + std::atomic_bool active; + std::atomic_bool running; + std::atomic<pthread_t> childTid; + ///< Assume that the content of childTid is valid when the class member + ///< active is set to true and undefined otherwise. tThreadId childThreadId; cMutex mutex; char *description; -- 2.39.1 [View Less]

3 4

[ANNOUNCE] VDR version 2.6.4 released
by Klaus Schmidinger 18 Feb '23

18 Feb '23

VDR version 2.6.4 is now available at the official VDR GIT archive git://git.tvdr.de You can also get the latest stable version with git clone --branch stable/2.6 git://git.tvdr.de/vdr.git or as a tar archive with http://git.tvdr.de/?p=vdr.git;a=snapshot;h=refs/tags/2.6.4;sf=tbz2 This version fixes a few bugs that came up after the release of version 2.6.3. The changes since version 2.6.3: - Updated the Italian OSD texts (thanks to Diego Pierotto). - Fixed restoring … [View More]

1 0

[PATCH] Use cThread::mutex with absolute cCondVar::TimedWait()
by Marko Mäkelä 18 Feb '23

18 Feb '23

Hi, I would propose the following patch, or some equivalent interface that would allow cThread::mutex to be used with some cCondVar in derived classes: diff --git a/thread.h b/thread.h index 16c4bd75..cd1d98ab 100644 --- a/thread.h +++ b/thread.h @@ -83,7 +83,9 @@ private: bool running; pthread_t childTid; tThreadId childThreadId; +protected: cMutex mutex; +private: char *description; bool lowPriority; static tThreadId mainThreadId; Because cThread::mutx is … [View More]declared private and there is no helper member function, derived classes that wish to use condition variables have to instantiate a separate cMutex for that. Here is an example from the rpihddevice plugin that illustrates my point: diff --git a/omx.c b/omx.c --- a/omx.c +++ b/omx.c @@ -119,17 +119,17 @@ const char* cOmx::errStr(int err) void cOmx::Action(void) { cTimeMs timer; - m_mutex.Lock(); + Lock(); while (Running()) { while (m_portEvents.empty()) - if (m_portEventsAdded.TimedWait(m_mutex, 10)) + if (!m_portEventsAdded.TimedWait(mutex, 10)) goto timeout; { const Event event = m_portEvents.front(); m_portEvents.pop(); - m_mutex.Unlock(); + Unlock(); switch (event.event) { Actually, there is a bug above: the condition for the TimedWait() call was inverted. This code illustrates another limitation: There is no way to pass an absolute time to cCondVar::TimedWait(). On each call, a relative wake-up time (milliseconds from the current time) will be converted into an absolute time. If there was a way, we would be able to remove the "cTimeMs timer" and some related system calls, and have this loop both wake up every 100 milliseconds, and process events as soon as they arrive. Here is the VDR part of the patch: diff --git a/thread.c b/thread.c index 93eb8c0d..3dcb44d4 100644 --- a/thread.c +++ b/thread.c @@ -36,7 +36,7 @@ #define dbglocking(a...) #endif -static bool GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow) +bool cCondVar::GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow) { struct timeval now; if (gettimeofday(&now, NULL) == 0) { // get current time @@ -81,7 +81,7 @@ bool cCondWait::Wait(int TimeoutMs) if (!signaled) { if (TimeoutMs) { struct timespec abstime; - if (GetAbsTime(&abstime, TimeoutMs)) { + if (cCondVar::GetAbsTime(&abstime, TimeoutMs)) { while (!signaled) { if (pthread_cond_timedwait(&cond, &mutex, &abstime) == ETIMEDOUT) break; @@ -129,20 +129,27 @@ void cCondVar::Wait(cMutex &Mutex) } } +bool cCondVar::TimedWait(cMutex &Mutex, const timespec &abstime) +{ + int err = 0; + + if (int locked = Mutex.locked) { + Mutex.locked = 0; // have to clear the locked count here, as pthread_cond_timedwait + // does an implicit unlock of the mutex. + err = pthread_cond_timedwait(&cond, &Mutex.mutex, &abstime); + Mutex.locked = locked; + } + return err != ETIMEDOUT; +} + bool cCondVar::TimedWait(cMutex &Mutex, int TimeoutMs) { bool r = true; // true = condition signaled, false = timeout if (Mutex.locked) { struct timespec abstime; - if (GetAbsTime(&abstime, TimeoutMs)) { - int locked = Mutex.locked; - Mutex.locked = 0; // have to clear the locked count here, as pthread_cond_timedwait - // does an implicit unlock of the mutex. - if (pthread_cond_timedwait(&cond, &Mutex.mutex, &abstime) == ETIMEDOUT) - r = false; - Mutex.locked = locked; - } + if (GetAbsTime(&abstime, TimeoutMs)) + r = TimedWait(Mutex, abstime); } return r; } @@ -174,7 +181,7 @@ bool cRwLock::Lock(bool Write, int TimeoutMs) int Result = 0; struct timespec abstime; if (TimeoutMs) { - if (!GetAbsTime(&abstime, TimeoutMs)) + if (!cCondVar::GetAbsTime(&abstime, TimeoutMs)) TimeoutMs = 0; } if (Write) { diff --git a/thread.h b/thread.h index 16c4bd75..04bb4cc5 100644 --- a/thread.h +++ b/thread.h @@ -49,7 +49,9 @@ public: ~cCondVar(); void Wait(cMutex &Mutex); bool TimedWait(cMutex &Mutex, int TimeoutMs); + bool TimedWait(cMutex &Mutex, const timespec &abstime); void Broadcast(void); + static bool GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow); }; class cRwLock { I did not complete the change to rpihddevice cOmx::Action() yet. We may be forced to retain two mutexes after all, because OMX_EmptyThisBuffer() as well as one of the functions called from cOmx::StopAudio() would hang indefinitely, while holding the cOmx::mutex, blocking the cOmx::Action() thread. According to https://forums.raspberrypi.com/viewtopic.php?t=313170 the interface is largely deprecated. I will try to figure out something. Best regards, Marko [View Less]

2 3

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

vdr February 2023