[mythtv-commits] Ticket #13414: Valgrind error in DVBStreamHandler / DeviceReadBuffer

[MythTV]

#13414: Valgrind error in DVBStreamHandler / DeviceReadBuffer
---------------------------------+-----------------------------------
     Reporter:  Klaas de Waal    |      Owner:  (none)
         Type:  Patch - Bug Fix  |     Status:  new
     Priority:  major            |  Milestone:  needs_triage
    Component:  MythTV - DVB     |    Version:  Master Head
     Severity:  medium           |   Keywords:  DVB segfault valgrind
Ticket locked:  0                |
---------------------------------+-----------------------------------
 Summary:\\
 Testing with valgrind shows sometimes a bug in the DVBStreamHandler about
 access of memory which has already been free'd. This happens at the end of
 a recording. Such an error can cause all kinds of errors, including
 segmentation faults.\\
 A fix for this is attached.\\

 Analysis:\\
 The problem lies in DVBStreamHandler::SetRunningDesired where first the
 m_running_desired flag is set to false and then the DeviceReadBuffer is
 stopped by calling _drb->Stop().

 {{{
 void DVBStreamHandler::SetRunningDesired(bool desired)
 {
     if (_drb && m_running_desired && !desired)
     {
         StreamHandler::SetRunningDesired(desired);
         _drb->Stop();
     }
     else
     {
         StreamHandler::SetRunningDesired(desired);
     }
 }

 }}}

 {{{
 void StreamHandler::SetRunningDesired(bool desired)
 {
     m_running_desired = desired;
     if (!desired)
         MThread::exit(0);
 }

 }}}

 When m_running_desired is false then DVBStreamHandler::RunTS, where the
 DeviceReadBuffer is created, deletes the DeviceReadBuffer after having set
 _drb to nullptr. This is in lines 293-303 of DVBStreamHandler::RunTS:

 {{{
     {
         QMutexLocker locker(&m_start_stop_lock);
         _drb = nullptr;
     }

     if (drb)
     {
         if (drb->IsRunning())
             drb->Stop();
         delete drb;
     }

 }}}

 Depending on the dynamics of a multi-threaded application and on compilers
 saving non-volatile values in registers, the value of _drb in
 DVBStreamHandler::SetRunningDesired() after calling
 StreamHandler::SetRunningDesired() can now be one of the following:
 - point to a valid DeviceReadBuffer
 - point to a free'd DeviceReadBuffer
 - zero/nullptr
 \\
 The pointer is still valid when RunTS has not yet deleted the
 DeviceReadBuffer; in this case everything is OK and there is no valgrind
 error message.\\

 When RunTS has already deleted the DeviceReadBuffer then the pointer
 points to a free'd DeviceReadBuffer and this is what causes
 the valgrind error message. Because the access is really soon after the
 free the memory content is usually still there so most of the time nothing
 problematic happens. When the memory has been re-used then anything can
 happen.\\

 When the pointer is also zero in SetRunningDesired then this causes a
 segmentation fault of the mythbackend.\\

 The hypotheses that there is a race condition between two threads can be
 tested by adding a 50 millisecond delay as follows:

 {{{
 void DVBStreamHandler::SetRunningDesired(bool desired)
 {
     if (_drb && m_running_desired && !desired)
     {
         StreamHandler::SetRunningDesired(desired);
         std::this_thread::sleep_for(std::chrono::milliseconds(50));     //
 KdW this triggers the segfault
         _drb->Stop();
     }
     else
     {
         StreamHandler::SetRunningDesired(desired);
     }
 }

 }}}
 The logic flow has not changed by this small delay but mythbackend now
 crashes reliably at end of the first recording.\\

 The least-intrusive fix for this is to stop the DeviceReadBuffer before it
 is deleted:

 {{{
 void DVBStreamHandler::SetRunningDesired(bool desired)
 {
     if (_drb && m_running_desired && !desired)
     {
         _drb->Stop();
         StreamHandler::SetRunningDesired(desired);
     }
     else
     {
         StreamHandler::SetRunningDesired(desired);
     }
 }

 }}}
 The patch for this is attached as 20190226-dvbsh-drb-v1.patch\\


 Examining the log files shows that the DeviceRequestBlock::Stop() function
 is called three times at the end of each recording:\\
 - First time is in DVBStreamHandler::SetRunningDesired()
 - Second time in DVBStreamHandler::RunTS() before the delete:

 {{{
    if (drb)
     {
         if (drb->IsRunning())
             drb->Stop();
         delete drb;
     }
 }}}
 - Third time in DeviceReadBuffer::~DeviceReadBuffer():

 {{{
 DeviceReadBuffer::~DeviceReadBuffer()
 {
     Stop();
     if (m_buffer)
     {
         delete[] m_buffer;
         m_buffer = nullptr;
     }
 }

 }}}

 Because the Stop() is called unconditionally in the destructor there is no
 need to call it in DVBStreamHandler::RunTS or in
 DVBStreamHandler::SetRunningDesired().\\

 The only thing DVBStreamHandler::SetRunningDesired() then does is call
 StreamHandler::SetRunningDesired(). The conclusion is that the
 DVBStreamHandler::SetRunningDesired() can be removed.\\

 The result, as tested on my system, is that the Stop() is called only once
 and that there are no valgrind messages anymore related to this.\\

 The patch for all this is attached as 20190226-dvbsh-drb-v5.patch.\\

-- 
Ticket URL: <https://code.mythtv.org/trac/ticket/13414>
MythTV <http://www.mythtv.org>
MythTV Media Center