Valgrind error in DVBStreamHandler / DeviceReadBuffer

[Klaas de Waal]

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.

[Klaas de Waal]

Minimal fix for dvbstreamhandler.cpp

[Klaas de Waal]

Full fix for dvbstreamhandler.cpp and dvbstreamhandler.h, removed duplicate calls, removed superfluous override function.

[Klaas de Waal]

One valgrind message from mb-20190225-1221-vg.log

[Klaas de Waal]

Complete valgrind log, mythtv-related messages start at line 2395

[Klaas de Waal <kdewaal@…>]

In 07cdda87a/mythtv:

Valgrind error in DVBStreamHandler / DeviceReadBuffer?

The DeviceReadBuffer? is now stopped only once at the end of recording
instead of three times. One of the two later stop attempts accessed memory
that was already free'd in a previous stop. This caused valgrind errors
when testing with valgrind and this caused also possibly occasional
segmentation faults of mythbackend at the end of a recording.

Fixes #13414