Opened 10 months ago

Closed 9 months ago

#13414 closed Patch - Bug Fix (fixed)

Valgrind error in DVBStreamHandler / DeviceReadBuffer

Reported by: Klaas de Waal Owned by: Klaas de Waal
Priority: major Milestone: 31.0
Component: MythTV - DVB Version: Master Head
Severity: medium Keywords: DVB segfault valgrind
Cc: Stuart Auchterlonie Ticket locked: no

Description

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:


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;
    }
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.

Attachments (4)

20190226-dvbsh-drb-v1.patch (559 bytes) - added by Klaas de Waal 10 months ago.
Minimal fix for dvbstreamhandler.cpp
20190226-dvbsh-drb-v5.patch (1.9 KB) - added by Klaas de Waal 10 months ago.
Full fix for dvbstreamhandler.cpp and dvbstreamhandler.h, removed duplicate calls, removed superfluous override function.
valgrindmessage.log (2.1 KB) - added by Klaas de Waal 10 months ago.
One valgrind message from mb-20190225-1221-vg.log
mb-20190225-1221-vg.log (205.5 KB) - added by Klaas de Waal 10 months ago.
Complete valgrind log, mythtv-related messages start at line 2395

Download all attachments as: .zip

Change History (7)

Changed 10 months ago by Klaas de Waal

Attachment: 20190226-dvbsh-drb-v1.patch added

Minimal fix for dvbstreamhandler.cpp

Changed 10 months ago by Klaas de Waal

Attachment: 20190226-dvbsh-drb-v5.patch added

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

Changed 10 months ago by Klaas de Waal

Attachment: valgrindmessage.log added

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

Changed 10 months ago by Klaas de Waal

Attachment: mb-20190225-1221-vg.log added

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

comment:1 Changed 10 months ago by Klaas de Waal

Owner: set to Klaas de Waal
Status: newaccepted

comment:2 Changed 9 months ago by Klaas de Waal

Milestone: needs_triage31.0

comment:3 Changed 9 months ago by Klaas de Waal <kdewaal@…>

Resolution: fixed
Status: acceptedclosed

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

Note: See TracTickets for help on using tickets.