From 14228d554c1984a62a825dffdb2a1888c5104a8e Mon Sep 17 00:00:00 2001 From: Stefan Seyfried Date: Fri, 20 Feb 2015 09:30:19 +0100 Subject: [PATCH] spark: fix race condition in cDemux cDemux destructor was racing with Read() which did lead to all sorts of nasty crashes because after poll returned, the dmx object could be gone and its memory replaced with totally different things. Origin commit data ------------------ Branch: master Commit: https://github.com/neutrino-images/ni-libstb-hal/commit/b67a0a7e44cfe4d3c7692bfa92b5db7cc3e37bfe Author: Stefan Seyfried Date: 2015-02-20 (Fri, 20 Feb 2015) ------------------ This commit was generated by Migit --- libspark/dmx.cpp | 38 ++++++++++++++++++++++++++++++++++---- libspark/dmx_lib.h | 1 + 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/libspark/dmx.cpp b/libspark/dmx.cpp index d3ba4e7..6f8a969 100644 --- a/libspark/dmx.cpp +++ b/libspark/dmx.cpp @@ -66,11 +66,13 @@ #include #include #include +#include +#include #include "dmx_lib.h" #include "lt_debug.h" -/* Ugh... see comment in destructor for details... */ #include "video_lib.h" +/* needed for getSTC... */ extern cVideo *videoDecoder; #define lt_debug(args...) _lt_debug(TRIPLE_DEBUG_DEMUX, this, args) @@ -119,9 +121,11 @@ static const char *devname[NUM_DEMUXDEV] = { /* did we already DMX_SET_SOURCE on that demux device? */ static bool init[NUM_DEMUXDEV] = { false, false, false }; -/* uuuugly */ -static int dmx_tp_count = 0; -#define MAX_TS_COUNT 1 +typedef struct dmx_pdata { + int last_source; + OpenThreads::Mutex *mutex; +} dmx_pdata; +#define P ((dmx_pdata *)pdata) cDemux::cDemux(int n) { @@ -137,6 +141,11 @@ cDemux::cDemux(int n) last_measure = 0; last_data = 0; last_source = -1; + + pdata = (void *)calloc(1, sizeof(dmx_pdata)); + P->last_source = -1; + P->mutex = new OpenThreads::Mutex; + dmx_type = DMX_INVALID; } cDemux::~cDemux() @@ -155,6 +164,12 @@ cDemux::~cDemux() */ if (dmx_type == DMX_VIDEO_CHANNEL) videoDecoder = NULL; + /* wait until Read() has released the mutex */ + (*P->mutex).lock(); + (*P->mutex).unlock(); + free(P->mutex); + free(pdata); + pdata = NULL; } bool cDemux::Open(DMX_CHANNEL_TYPE pes_type, void * /*hVideoBuffer*/, int uBufferSize) @@ -250,6 +265,7 @@ void cDemux::Close(void) bool cDemux::Start(bool) { + lt_debug("%s #%d fd: %d type: %s\n", __func__, num, fd, DMX_T[dmx_type]); if (fd < 0) { lt_info("%s #%d: not open!\n", __FUNCTION__, num); @@ -261,6 +277,7 @@ bool cDemux::Start(bool) bool cDemux::Stop(void) { + lt_debug("%s #%d fd: %d type: %s\n", __func__, num, fd, DMX_T[dmx_type]); if (fd < 0) { lt_info("%s #%d: not open!\n", __FUNCTION__, num); @@ -282,6 +299,8 @@ int cDemux::Read(unsigned char *buff, int len, int timeout) lt_info("%s #%d: not open!\n", __func__, num); return -1; } + /* avoid race in destructor: ~cDemux needs to wait until Read() returns */ + OpenThreads::ScopedLock m_lock(*P->mutex); int rc; int to = timeout; struct pollfd ufds; @@ -299,6 +318,12 @@ int cDemux::Read(unsigned char *buff, int len, int timeout) { retry: rc = ::poll(&ufds, 1, to); + if (ufds.fd != fd) + { + /* Close() will set fd to -1, this is normal. Everything else is not. */ + lt_info("%s:1 ========== fd has changed, %d->%d ==========\n", __func__, ufds.fd, fd); + return -1; + } if (!rc) { if (timeout == 0) /* we took the emergency exit */ @@ -336,6 +361,11 @@ int cDemux::Read(unsigned char *buff, int len, int timeout) return 0; } } + if (ufds.fd != fd) /* does this ever happen? and if, is it harmful? */ + { /* read(-1,...) will just return EBADF anyway... */ + lt_info("%s:2 ========== fd has changed, %d->%d ==========\n", __func__, ufds.fd, fd); + return -1; + } rc = ::read(fd, buff, len); //fprintf(stderr, "fd %d ret: %d\n", fd, rc); diff --git a/libspark/dmx_lib.h b/libspark/dmx_lib.h index af87a02..056cb71 100644 --- a/libspark/dmx_lib.h +++ b/libspark/dmx_lib.h @@ -42,6 +42,7 @@ class cDemux struct dmx_pes_filter_params p_flt; int last_source; bool _open(void); + void *pdata; public: bool Open(DMX_CHANNEL_TYPE pes_type, void * unused = NULL, int bufsize = 0);