From c370f513cce8f4bb01d7bada7a10e2dcfeae3e24 Mon Sep 17 00:00:00 2001
From: Klaas de Waal <klaas@kldo.nl>
Date: Sat, 13 Aug 2022 18:37:55 +0200
Subject: [PATCH] Revert "Potential memory corruption problems in
 audiooutputbase"

This reverts commit 81aeb49c456ff4ecab82c2297b22c38bd70d706d.

Rationale:
This commit creates problems with recognition of 5.1 audio devices, see the thread
in mythtv-users "Digital audio Capability grayed out" from James Abernathy.

The problem is that the field "Digital Audio Capabilities" is always disabled
(shown as grey and not accessible) independent of the selected audio device.
Correct behavior is that when a digital output such as HDMI is selected that
it is then possible to select the capabilities of that digital output.
This is not possible when the field is disabled.

I see the following two changes in this commit:
Around line 198 the "bugfix: don't allocate".
The original code creates a copy of the complete struct pointed to by m_outputSettingsDigitalRaw; then
the Users are added and that is then saved in m_outputSettingsDigital.
This means the m_outputSettingsDigitalRaw and m_outputSettingsDigital are always different addresses.

The "bugfix: don't allocate" code changes this by copying the pointer value from m_outputSettingsDigitalRaw, then
the Users are added and then it is saved in m_outputSettingsDigital.

This does change two things:
- The m_outputSettingsDigitalRaw and m_outputSettingsDigital now point to the same struct, so the pointer values are the same.
- The m_outputSettingsDigitalRaw has been modified by adding the users, so it is not the Raw data anymore.
Note that if the idea is that m_outputSettingsDigital and m_outputSettingsDigitalRaw point to the same data then one pointer would be enough.
My conclusion is that this must be the change that causes the device recognition problems.

The "bugfix: don't allocate" code then causes problems in the destructor where now m_outputSettingsDigitalRaw
and m_outputSettingsDigital point to the same object and have the same value.
To protect against twice deleting the same memory the code starting at line 93
"// These all seem to be the same pointer, avoid freeing multiple times"
has been added.

To summarize my analysis:
- The "bugfix: don't allocate" code does create the problem in device recognition by copying a pointer instead of copying a struct.
- The fix in the destructor is only needed as a consequence of this.

Reverting the commit and then checking the behavior of the code with valgrind does not show memory corruption problems.
---
 mythtv/libs/libmyth/audio/audiooutputbase.cpp | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/mythtv/libs/libmyth/audio/audiooutputbase.cpp b/mythtv/libs/libmyth/audio/audiooutputbase.cpp
index c047226d39a..cc29e4941b3 100644
--- a/mythtv/libs/libmyth/audio/audiooutputbase.cpp
+++ b/mythtv/libs/libmyth/audio/audiooutputbase.cpp
@@ -92,18 +92,13 @@ AudioOutputBase::~AudioOutputBase()
                 "~AudioOutputBase called, but KillAudio has not been called!");
 
     // We got this from a subclass, delete it
-    // These all seem to be the same pointer, avoid freeing multiple times
-    VBAUDIO(QString("m_outputSettings != m_outputSettingsDigital : %1").arg(m_outputSettings != m_outputSettingsDigital));
-    if (m_outputSettingsDigital != m_outputSettings)
+    delete m_outputSettings;
+    delete m_outputSettingsRaw;
+    if (m_outputSettings != m_outputSettingsDigital)
     {
-        if (m_outputSettingsDigitalRaw != m_outputSettingsDigital && m_outputSettingsDigitalRaw != m_outputSettings)
-            delete m_outputSettingsDigitalRaw;
         delete m_outputSettingsDigital;
+        delete m_outputSettingsDigitalRaw;
     }
-    VBAUDIO(QString("m_outputSettingsRaw != m_outputSettings %1").arg(m_outputSettingsRaw != m_outputSettings));
-    if (m_outputSettingsRaw != m_outputSettings)
-        delete m_outputSettingsRaw;
-    delete m_outputSettings;
 
     if (m_kAudioSRCOutputSize > 0)
         delete[] m_srcOut;
@@ -197,9 +192,9 @@ AudioOutputSettings* AudioOutputBase::GetOutputSettingsUsers(bool digital)
     else if (m_outputSettingsDigital)
         return m_outputSettingsDigital;
 
-    //bugfix: don't allocate as GetOutputSettings will do it
-    //auto* aosettings = new AudioOutputSettings;
-    AudioOutputSettings* aosettings = GetOutputSettingsCleaned(digital);
+    auto* aosettings = new AudioOutputSettings;
+
+    *aosettings = *GetOutputSettingsCleaned(digital);
     aosettings->GetUsers();
 
     if (digital)