Re: [vdr] [PATCH] Add thread safety to cRingBufferLinear
On 02/02/2023 23:27, Klaus Schmidinger wrote: On 02.02.23 23:16, Patrick Lerda wrote: Beside preventing crashes with vdr-2.6.3 this is required to get vdr to work properly with the gcc thread sanitizer. cRingBufferLinear was designed to be thread safe without locking. What "crashes with vdr-2.6.3" are you referring to? Klaus With a -fsanitize=thread compiled version of vdr, I had some crashes that happened quickly, for instance: ThreadSanitizer:DEADLYSIGNAL ==6496==ERROR: ThreadSanitizer: SEGV on unknown address 0x7f6ca48884a4 (pc 0x7f6cbf45726d bp 0x7f6ca3dbfb40 sp 0x7f6ca3dbf038 T7926) ==6496==The signal is caused by a WRITE memory access. #0 (libc.so.6+0x16926d) #1 __interceptor_memcpy (libtsan.so.0+0x619fc) #2 cRingBufferLinear::Put(unsigned char const*, int) /dosy/src/vdr-2.6.3/ringbuffer.c:329 (vdr+0x5724ab) #3 cRemuxDummy::Put(unsigned char const*, int) /dosy/src/vdr-2.6.3/PLUGINS/src/streamdev/server/streamer.c:117 (libvdr-streamdev-server.so.2.6.3+0x504cd) #4 cStreamdevStreamer::Put(unsigned char const*, int) /dosy/src/vdr-2.6.3/PLUGINS/src/streamdev/server/streamer.c:182 (libvdr-streamdev-server.so.2.6.3+0x504cd) #5 cStreamdevLiveStreamer::Put(unsigned char const*, int) /dosy/src/vdr-2.6.3/PLUGINS/src/streamdev/server/livestreamer.c:604 (libvdr-streamdev-server.so.2.6.3+0x55565) #6 cStreamdevStreamer::Action() /dosy/src/vdr-2.6.3/PLUGINS/src/streamdev/server/streamer.c:174 (libvdr-streamdev-server.so.2.6.3+0x50718) #7 cStreamdevLiveStreamer::Action() /dosy/src/vdr-2.6.3/PLUGINS/src/streamdev/server/livestreamer.c:589 (libvdr-streamdev-server.so.2.6.3+0x55495) #8 cThread::StartThread(cThread*) /dosy/src/vdr-2.6.3/thread.c:293 (vdr+0x5be852) #9 (libtsan.so.0+0x333bf) #10 (libc.so.6+0x8a729) #11 (libc.so.6+0x1070bb) Here are two detected data races: WARNING: ThreadSanitizer: data race (pid=6496) Write of size 4 at 0x7b443084 by thread T24: #0 cRingBufferLinear::Put(unsigned char const*, int) vdr-2.6.3/ringbuffer.c:330 (vdr+0x5724c6) #1 cIptvDevice::WriteData(unsigned char*, int) vdr-2.6.3/PLUGINS/src/iptv/device.c:384 (libvdr-iptv.so.2.6.3+0x16384) #2 cIptvStreamer::Action() vdr-2.6.3/PLUGINS/src/iptv/streamer.c:51 (libvdr-iptv.so.2.6.3+0x2bc46) #3 cThread::StartThread(cThread*) vdr-2.6.3/thread.c:293 (vdr+0x5be852) #4 (libtsan.so.0+0x333bf) Previous read of size 4 at 0x7b443084 by thread T23: #0 cRingBufferLinear::Get(int&) vdr-2.6.3/ringbuffer.c:348 (vdr+0x572534) #1 cIptvDevice::GetData(int*) vdr-2.6.3/PLUGINS/src/iptv/device.c:408 (libvdr-iptv.so.2.6.3+0x1772c) #2 cIptvDevice::GetTSPacket(unsigned char*&) vdr-2.6.3/PLUGINS/src/iptv/device.c:456 (libvdr-iptv.so.2.6.3+0x17a21) #3 cDevice::Action() vdr-2.6.3/device.c:1718 (vdr+0x4bf86f) #4 cThread::StartThread(cThread*) vdr-2.6.3/thread.c:293 (vdr+0x5be852) #5 (libtsan.so.0+0x333bf) Location is heap block of size 288 at 0x7b442f80 allocated by main thread: #0 operator new(unsigned long) (libtsan.so.0+0x8d98c) #1 cIptvDevice::cIptvDevice(unsigned int) vdr-2.6.3/PLUGINS/src/iptv/device.c:29 (libvdr-iptv.so.2.6.3+0x183b4) #2 cIptvDevice::Initialize(unsigned int) vdr-2.6.3/PLUGINS/src/iptv/device.c:88 (libvdr-iptv.so.2.6.3+0x188b7) #3 cPluginIptv::Initialize() vdr-2.6.3/PLUGINS/src/iptv/iptv.c:109 (libvdr-iptv.so.2.6.3+0x140f2) #4 cPluginManager::InitializePlugins() vdr-2.6.3/plugin.c:375 (vdr+0x5536f2) #5 main vdr-2.6.3/vdr.c:825 (vdr+0x490ccb) Thread T24 'IPTV streamer' (tid=6535, running) created by thread T23 at: #0 pthread_create (libtsan.so.0+0x5fea5) #1 cThread::Start() vdr-2.6.3/thread.c:327 (vdr+0x5be393) #2 cIptvStreamer::Open() vdr-2.6.3/PLUGINS/src/iptv/streamer.c:66 (libvdr-iptv.so.2.6.3+0x2bf46) #3 cIptvDevice::OpenDvr() vdr-2.6.3/PLUGINS/src/iptv/device.c:342 (libvdr-iptv.so.2.6.3+0x15ed3) #4 cDevice::Action() vdr-2.6.3/device.c:1714 (vdr+0x4bf7b8) #5 cThread::StartThread(cThread*) vdr-2.6.3/thread.c:293 (vdr+0x5be852) #6 (libtsan.so.0+0x333bf) Thread T23 'device 3 receiv' (tid=6534, running) created by thread T13 at: #0 pthread_create (libtsan.so.0+0x5fea5) #1 cThread::Start() vdr-2.6.3/thread.c:327 (vdr+0x5be393) #2 cDevice::AttachReceiver(cReceiver*) vdr-2.6.3/device.c:1844 (vdr+0x4c055d) #3 cStreamdevLiveStreamer::Attach() vdr-2.6.3/PLUGINS/src/streamdev/server/livestreamer.c:614 (libvdr-streamdev-server.so.2.6.3+0x51761) #4 cStreamdevStreamer::Start(cTBSocket*) vdr-2.6.3/PLUGINS/src/streamdev/server/streamer.c:152 (libvdr-streamdev-server.so.2.6.3+0x508f4) #5 cConnectionHTTP::Flushed() vdr-2.6.3/PLUGINS/src/streamdev/server/connectionHTTP.c:439 (libvdr-streamdev-server.so.2.6.3+0x38e33) #6 cServerConnection::Write() vdr-2.6.3/PLUGINS/src/streamdev/server/connection.c:171 (libvdr-streamdev-server.so.2.6.3+0x2d1bf) #7 cStreamdevServer::Action()
Re: [vdr] [PATCH] Add thread safety to cRingBufferLinear
On 02.02.23 23:16, Patrick Lerda wrote: Beside preventing crashes with vdr-2.6.3 this is required to get vdr to work properly with the gcc thread sanitizer. cRingBufferLinear was designed to be thread safe without locking. What "crashes with vdr-2.6.3" are you referring to? Klaus ___ vdr mailing list vdr@linuxtv.org https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
[vdr] [PATCH] Add thread safety to cRingBufferLinear
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 = 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 )
{
+ Lock();
int Head = head;
if (getThreadTid <= 0)
getThreadTid = cThread::ThreadId();
@@ -362,14 +374,17 @@ uchar *cRingBufferLinear::Get(int )
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
___
vdr mailing list
vdr@linuxtv.org
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
[vdr] [PATCH] Fix cThread related race conditions
This change fixes the following issue:
==15457==ERROR: AddressSanitizer: SEGV on unknown address 0x02d0 (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 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(, NULL, (void *(*) (void *)),
(void *)this) == 0) {
- pthread_detach(childTid); // auto-reap
+pthread_t localTid;
+running = true;
+if (pthread_create(, NULL, (void *(*) (void *)),
(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
#include
#include
+#include
typedef pid_t tThreadId;
@@ -56,7 +57,7 @@ class cRwLock {
private:
pthread_rwlock_t rwlock;
int locked;
- tThreadId writeLockThreadId;
+ std::atomic 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 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
___
vdr mailing list
vdr@linuxtv.org
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
