diff mbox series

Race conditions on shutdown

Message ID Y4tbIOCmuPV2AdAu@jyty
State New
Headers
Series Race conditions on shutdown |

Commit Message

Marko Mäkelä Dec. 3, 2022, 2:20 p.m. UTC 
  Wed, Nov 30, 2022 at 02:01:13PM +0100, Klaus Schmidinger wrote:
>VDR version 2.6.2 is now available at the official VDR GIT archive
>
>      git://git.tvdr.de

Thank you, Klaus!

While debugging hangs or crashes during the shutdown of rpihddevice (see 
https://github.com/reufer/rpihddevice/pull/6 for my fixes), I noticed 
that AddressSanitizer (enabled by -fsanitize=address in GCC or clang) 
would report some heap-use-after-free also in
cDvbTuner::Action(), accessing a deleted cSdtFilter.

The attached patch fixes the issues for me. The Cancel(3) call in 
cDevice::~cDevice() is moved to cDevice::Shutdown(), because 
cDevice::~cDevice() would be executed too late, while 
cDvbDevice::~cDvbDevice() had already deleted the objects. 

The Cancel(-1) calls in cDevice::Shutdown() are an optimization to 
reduce the shutdown time on systems when there are multiple devices.

The change to cDvbDevice::~cDvbDevice() is necessary to shut down all 
users of cDvbTuner before it is deleted. Without this last hunk, I would 
still occasionally get heap-use-after-free reports.

	Marko
diff mbox series

Patch

diff --git a/device.c b/device.c
index 4b9c9cc7..4e987389 100644
--- a/device.c
+++ b/device.c
@@ -125,7 +125,6 @@  cDevice::~cDevice()
   delete dvbSubtitleConverter;
   if (this == primaryDevice)
      primaryDevice = NULL;
-  Cancel(3);
 }
 
 bool cDevice::WaitForAllDevicesReady(int Timeout)
@@ -457,7 +456,10 @@  void cDevice::SetCamSlot(cCamSlot *CamSlot)
 void cDevice::Shutdown(void)
 {
   deviceHooks.Clear();
+  for (int i = 0; i < numDevices; i++)
+      device[i]->Cancel(-1);
   for (int i = 0; i < numDevices; i++) {
+      device[i]->Cancel(3);
       delete device[i];
       device[i] = NULL;
       }
diff --git a/dvbdevice.c b/dvbdevice.c
index 51331485..1caf1218 100644
--- a/dvbdevice.c
+++ b/dvbdevice.c
@@ -573,6 +573,7 @@  private:
   virtual void Action(void);
 public:
   cDvbTuner(const cDvbDevice *Device, int Adapter, int Frontend);
+  void Shutdown();
   virtual ~cDvbTuner();
   bool ProvidesDeliverySystem(int DeliverySystem) const;
   bool ProvidesModulation(int System, int StreamId, int Modulation) const;
@@ -648,12 +649,17 @@  cDvbTuner::cDvbTuner(const cDvbDevice *Device, int Adapter, int Frontend)
   Start();
 }
 
-cDvbTuner::~cDvbTuner()
+void cDvbTuner::Shutdown()
 {
   tunerStatus = tsIdle;
   newSet.Broadcast();
   locked.Broadcast();
   Cancel(3);
+}
+
+cDvbTuner::~cDvbTuner()
+{
+  Shutdown();
   UnBond();
   /* looks like this irritates the SCR switch, so let's leave it out for now
   if (lastDiseqc && lastDiseqc->IsScr()) {
@@ -1865,6 +1871,8 @@  cDvbDevice::cDvbDevice(int Adapter, int Frontend)
 
 cDvbDevice::~cDvbDevice()
 {
+  if (dvbTuner)
+     dvbTuner->Shutdown();
   StopSectionHandler();
   delete dvbTuner;
   delete ciAdapter;