Opened 5 years ago
Closed 5 years 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:
- 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.
Attachments (4)
Change History (7)
Changed 5 years ago by
Attachment: | 20190226-dvbsh-drb-v1.patch added |
---|
Changed 5 years ago by
Attachment: | 20190226-dvbsh-drb-v5.patch added |
---|
Full fix for dvbstreamhandler.cpp and dvbstreamhandler.h, removed duplicate calls, removed superfluous override function.
Changed 5 years ago by
Attachment: | valgrindmessage.log added |
---|
One valgrind message from mb-20190225-1221-vg.log
Changed 5 years ago by
Attachment: | mb-20190225-1221-vg.log added |
---|
Complete valgrind log, mythtv-related messages start at line 2395
comment:1 Changed 5 years ago by
Owner: | set to Klaas de Waal |
---|---|
Status: | new → accepted |
comment:2 Changed 5 years ago by
Milestone: | needs_triage → 31.0 |
---|
Minimal fix for dvbstreamhandler.cpp