Fixed a possible crash in case replay is started and stopped in rapid sequence2.4.2

7 files changed, 92 insertions, 39 deletions

diff --git a/HISTORY b/HISTORY
index 78a8bdab..edd56ac5 100644
--- a/HISTORY
+++ b/HISTORY
@@ -9420,7 +9420,7 @@ Video Disk Recorder Revision History
 - Fixed handling the S2SatelliteDeliverySystemDescriptor for transponders broadcasting
   in "backwards compatibility mode" according to ETSI EN 300 468 (thanks to Onur Sentürk).
 
-2020-05-15:
+2020-05-18: Version 2.4.2
 
 - Fixed moving channels between number groups in SVDRP's MOVC command and the Channels
   menu, in case a channel is moved to a higher number and into a numbered group
@@ -9443,3 +9443,12 @@ Video Disk Recorder Revision History
   is valid (suggested by Helmut Binder).
 - The isSingleByte parameter in the call to getCharacterTable() is deprecated and only
   present for backwards compatibility.
+- Fixed a possible crash in case replay is started and stopped in rapid sequence, by
+  adding missing locking to cControl::Control(). The caller of this function must now
+  provide a cMutexLock which stays alive as long as the result of this call is used.
+  The old version of this function is still there for backwards compatibility with
+  plugins, because this problem appears to occur only under very rare circumstances.
+  Authors of plugins that use this function should switch to the new version, because
+  the old one is deprecated and will be removed in a future version.
+  The version numbers (both VDRVERSNUM and APIVERSNUM) have been bumped to 2.4.2, so
+  that plugins can detect the presence of the new cControl::Control().
diff --git a/config.h b/config.h
index 4f37e072..72be5231 100644
--- a/config.h
+++ b/config.h
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: config.h 4.16 2019/06/16 09:13:45 kls Exp $
+ * $Id: config.h 4.17 2020/05/18 16:47:29 kls Exp $
  */
 
 #ifndef __CONFIG_H
@@ -22,13 +22,13 @@
 
 // VDR's own version number:
 
-#define VDRVERSION  "2.4.1"
-#define VDRVERSNUM   20401  // Version * 10000 + Major * 100 + Minor
+#define VDRVERSION  "2.4.2"
+#define VDRVERSNUM   20402  // Version * 10000 + Major * 100 + Minor
 
 // The plugin API's version number:
 
-#define APIVERSION  "2.4.1"
-#define APIVERSNUM   20401  // Version * 10000 + Major * 100 + Minor
+#define APIVERSION  "2.4.2"
+#define APIVERSNUM   20402  // Version * 10000 + Major * 100 + Minor
 
 // When loading plugins, VDR searches them by their APIVERSION, which
 // may be smaller than VDRVERSION in case there have been no changes to
diff --git a/menu.c b/menu.c
index 58c7763e..baa93173 100644
--- a/menu.c
+++ b/menu.c
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: menu.c 4.81 2020/04/11 09:22:05 kls Exp $
+ * $Id: menu.c 4.82 2020/05/18 16:47:29 kls Exp $
  */
 
 #include "menu.h"
@@ -2731,7 +2731,8 @@ eOSState cMenuRecordingEdit::DeleteMarks(void)
   if (buttonDeleteMarks && Interface->Confirm(tr("Delete editing marks for this recording?"))) {
      if (cMarks::DeleteMarksFile(recording)) {
         SetHelpKeys();
-        if (cControl *Control = cControl::Control(true)) {
+        cMutexLock ControlMutexLock;
+        if (cControl *Control = cControl::Control(ControlMutexLock, true)) {
            if (const cRecording *Recording = Control->GetRecording()) {
               if (strcmp(recording->FileName(), Recording->FileName()) == 0)
                  Control->ClearEditingMarks();
@@ -4457,7 +4458,11 @@ bool cMenuMain::Update(bool Force)
 {
   bool result = false;
 
-  bool NewReplaying = cControl::Control() != NULL;
+  bool NewReplaying = false;
+  {
+    cMutexLock ControlMutexLock;
+    NewReplaying = cControl::Control(ControlMutexLock) != NULL;
+  }
   if (Force || NewReplaying != replaying) {
      replaying = NewReplaying;
      // Replay control:
diff --git a/player.c b/player.c
index 5c95f4e4..ff54f495 100644
--- a/player.c
+++ b/player.c
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: player.c 2.2 2012/04/28 11:52:50 kls Exp $
+ * $Id: player.c 4.1 2020/05/18 16:47:29 kls Exp $
  */
 
 #include "player.h"
@@ -70,16 +70,24 @@ cString cControl::GetHeader(void)
   return "";
 }
 
+#if DEPRECATED_CCONTROL
 cControl *cControl::Control(bool Hidden)
 {
   cMutexLock MutexLock(&mutex);
   return (control && (!control->hidden || Hidden)) ? control : NULL;
 }
+#endif
+
+cControl *cControl::Control(cMutexLock &MutexLock, bool Hidden)
+{
+  MutexLock.Lock(&mutex);
+  return (control && (!control->hidden || Hidden)) ? control : NULL;
+}
 
 void cControl::Launch(cControl *Control)
 {
   cMutexLock MutexLock(&mutex);
-  cControl *c = control; // keeps control from pointing to uninitialized memory
+  cControl *c = control; // keeps control from pointing to uninitialized memory TODO obsolete once DEPRECATED_CCONTROL is gone
   control = Control;
   delete c;
 }
diff --git a/player.h b/player.h
index d67bf2a9..22c748bd 100644
--- a/player.h
+++ b/player.h
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: player.h 4.4 2018/02/01 15:34:51 kls Exp $
+ * $Id: player.h 4.5 2020/05/18 16:47:29 kls Exp $
  */
 
 #ifndef __PLAYER_H
@@ -114,10 +114,21 @@ public:
   static void Launch(cControl *Control);
   static void Attach(void);
   static void Shutdown(void);
+#define DEPRECATED_CCONTROL 1
+#if DEPRECATED_CCONTROL
   static cControl *Control(bool Hidden = false);
+         ///< Old version of this function, for backwards compatibility with plugins.
+         ///< Plugins should be changed to use the new version below, which does
+         ///< proper locking.
+         ///< Use of this function may result in program crashes in case replay is
+         ///< stopped immediately after starting it.
+#endif
+  static cControl *Control(cMutexLock &MutexLock, bool Hidden = false);
          ///< Returns the current replay control (if any) in case it is currently
          ///< visible. If Hidden is true, the control will be returned even if it is
          ///< currently hidden.
+         ///< The given MutexLock must live as long as the replay control is accessed,
+         ///< and must go out of scope as soon as the control is no longer accessed.
   };
 
 #endif //__PLAYER_H
diff --git a/skinlcars.c b/skinlcars.c
index aedca5dc..0f431631 100644
--- a/skinlcars.c
+++ b/skinlcars.c
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: skinlcars.c 4.6 2017/11/08 10:10:30 kls Exp $
+ * $Id: skinlcars.c 4.7 2020/05/18 16:47:29 kls Exp $
  */
 
 // "Star Trek: The Next Generation"(R) is a registered trademark of Paramount Pictures,
@@ -1741,12 +1741,13 @@ void cSkinLCARSDisplayMenu::Flush(void)
 {
   if (MenuCategory() == mcMain) {
      cDevice *Device = cDevice::PrimaryDevice();
+     cMutexLock ControlMutexLock;
      if (!Device->Replaying() || Device->Transferring()) {
         LOCK_CHANNELS_READ;
         const cChannel *Channel = Channels->GetByNumber(cDevice::PrimaryDevice()->CurrentChannel());
         DrawLive(Channel);
         }
-     else if (cControl *Control = cControl::Control(true))
+     else if (cControl *Control = cControl::Control(ControlMutexLock, true))
         DrawPlay(Control);
      DrawTimers();
      DrawDevices();
diff --git a/vdr.c b/vdr.c
index 77f65673..2ff5cfb4 100644
--- a/vdr.c
+++ b/vdr.c
@@ -22,7 +22,7 @@
  *
  * The project's page is at http://www.tvdr.de
  *
- * $Id: vdr.c 4.32 2020/05/15 11:31:40 kls Exp $
+ * $Id: vdr.c 4.33 2020/05/18 16:47:29 kls Exp $
  */
 
 #include <getopt.h>
@@ -1196,8 +1196,19 @@ int main(int argc, char *argv[])
         // Queued messages:
         Skins.ProcessQueuedMessages();
         // User Input:
-        cOsdObject *Interact = Menu ? Menu : cControl::Control();
-        eKeys key = Interface->GetKey(!Interact || !Interact->NeedsFastResponse());
+        bool NeedsFastResponse = Menu && Menu->NeedsFastResponse();
+        if (!NeedsFastResponse) {
+           // Must limit the scope of ControlMutexLock here to not hold the lock during the call to Interface->GetKey().
+           cMutexLock ControlMutexLock;
+           cControl *Control = cControl::Control(ControlMutexLock);
+           NeedsFastResponse = Control && Control->NeedsFastResponse();
+           }
+        eKeys key = Interface->GetKey(!NeedsFastResponse);
+        cOsdObject *Interact = Menu;
+        cMutexLock ControlMutexLock;
+        cControl *Control = NULL;
+        if (!Menu)
+           Interact = Control = cControl::Control(ControlMutexLock);
         if (ISREALKEY(key)) {
            EITScanner.Activity();
            // Cancel shutdown countdown:
@@ -1215,9 +1226,9 @@ int main(int argc, char *argv[])
                bool WasMenu = Interact && Interact->IsMenu();
                if (Menu)
                   DELETE_MENU;
-               else if (cControl::Control()) {
+               else if (Control) {
                   if (cOsd::IsOpen())
-                     cControl::Control()->Hide();
+                     Control->Hide();
                   else
                      WasOpen = false;
                   }
@@ -1233,9 +1244,9 @@ int main(int argc, char *argv[])
                   }
                else if (!Menu) {
                   IsInfoMenu = true;
-                  if (cControl::Control()) {
-                     cControl::Control()->Hide();
-                     Menu = cControl::Control()->GetInfo();
+                  if (Control) {
+                     Control->Hide();
+                     Menu = Control->GetInfo();
                      if (Menu)
                         Menu->Show();
                      else
@@ -1252,8 +1263,8 @@ int main(int argc, char *argv[])
           // Direct main menu functions:
           #define DirectMainFunction(function)\
             { DELETE_MENU;\
-            if (cControl::Control())\
-               cControl::Control()->Hide();\
+            if (Control)\
+               Control->Hide();\
             Menu = new cMenuMain(function);\
             key = kNone; } // nobody else needs to see this key
           case kSchedule:   DirectMainFunction(osSchedule); break;
@@ -1267,8 +1278,8 @@ int main(int argc, char *argv[])
                const char *PluginName = cRemote::GetPlugin();
                if (PluginName) {
                   DELETE_MENU;
-                  if (cControl::Control())
-                     cControl::Control()->Hide();
+                  if (Control)
+                     Control->Hide();
                   cPlugin *plugin = cPluginManager::GetPlugin(PluginName);
                   if (plugin) {
                      Menu = plugin->MainMenuAction();
@@ -1290,7 +1301,7 @@ int main(int argc, char *argv[])
                   Menu = new cDisplayChannel(NORMALKEY(key));
                   continue;
                   }
-               else if (cDisplayChannel::IsOpen() || cControl::Control()) {
+               else if (cDisplayChannel::IsOpen() || Control) {
                   Interact->ProcessKey(key);
                   continue;
                   }
@@ -1318,8 +1329,8 @@ int main(int argc, char *argv[])
                break;
           // Audio track control:
           case kAudio:
-               if (cControl::Control())
-                  cControl::Control()->Hide();
+               if (Control)
+                  Control->Hide();
                if (!cDisplayTracks::IsOpen()) {
                   DELETE_MENU;
                   Menu = cDisplayTracks::Create();
@@ -1330,8 +1341,8 @@ int main(int argc, char *argv[])
                break;
           // Subtitle track control:
           case kSubtitles:
-               if (cControl::Control())
-                  cControl::Control()->Hide();
+               if (Control)
+                  Control->Hide();
                if (!cDisplaySubtitleTracks::IsOpen()) {
                   DELETE_MENU;
                   Menu = cDisplaySubtitleTracks::Create();
@@ -1343,7 +1354,7 @@ int main(int argc, char *argv[])
           // Pausing live video:
           case kPlayPause:
           case kPause:
-               if (!cControl::Control()) {
+               if (!Control) {
                   DELETE_MENU;
                   if (Setup.PauseKeyHandling) {
                      if (Setup.PauseKeyHandling > 1 || Interface->Confirm(tr("Pause live video?"))) {
@@ -1356,7 +1367,7 @@ int main(int argc, char *argv[])
                break;
           // Instant recording:
           case kRecord:
-               if (!cControl::Control()) {
+               if (!Control) {
                   if (Setup.RecordKeyHandling) {
                      if (Setup.RecordKeyHandling > 1 || Interface->Confirm(tr("Start recording?"))) {
                         if (cRecordControls::Start())
@@ -1395,15 +1406,16 @@ int main(int argc, char *argv[])
                break;
           default: break;
           }
-        Interact = Menu ? Menu : cControl::Control(); // might have been closed in the mean time
+        Interact = Menu ? Menu : Control; // might have been closed in the mean time
         if (Interact) {
            LastInteract = Now;
            eOSState state = Interact->ProcessKey(key);
-           if (state == osUnknown && Interact != cControl::Control()) {
-              if (ISMODELESSKEY(key) && cControl::Control()) {
-                 state = cControl::Control()->ProcessKey(key);
+           if (state == osUnknown && Interact != Control) {
+              if (ISMODELESSKEY(key) && Control) {
+                 state = Control->ProcessKey(key);
                  if (state == osEnd) {
                     // let's not close a menu when replay ends:
+                    Control = NULL;
                     cControl::Shutdown();
                     continue;
                     }
@@ -1422,15 +1434,18 @@ int main(int argc, char *argv[])
                             break;
              case osRecordings:
                             DELETE_MENU;
+                            Control = NULL;
                             cControl::Shutdown();
                             Menu = new cMenuMain(osRecordings, true);
                             break;
              case osReplay: DELETE_MENU;
+                            Control = NULL;
                             cControl::Shutdown();
                             cControl::Launch(new cReplayControl);
                             break;
              case osStopReplay:
                             DELETE_MENU;
+                            Control = NULL;
                             cControl::Shutdown();
                             break;
              case osPlugin: DELETE_MENU;
@@ -1441,8 +1456,10 @@ int main(int argc, char *argv[])
              case osBack:
              case osEnd:    if (Interact == Menu)
                                DELETE_MENU;
-                            else
+                            else {
+                               Control = NULL;
                                cControl::Shutdown();
+                               }
                             break;
              default:       ;
              }
@@ -1487,6 +1504,7 @@ int main(int argc, char *argv[])
              // Instant resume of the last viewed recording:
              case kPlay:
                   if (cReplayControl::LastReplayed()) {
+                     Control = NULL;
                      cControl::Shutdown();
                      cControl::Launch(new cReplayControl);
                      }
@@ -1512,6 +1530,7 @@ int main(int argc, char *argv[])
         int NewPrimaryDVB = Setup.PrimaryDVB;
         if (NewPrimaryDVB != OldPrimaryDVB) {
            DELETE_MENU;
+           Control = NULL;
            cControl::Shutdown();
            Skins.QueueMessage(mtInfo, tr("Switching primary DVB..."));
            cOsdProvider::Shutdown();
@@ -1532,7 +1551,7 @@ int main(int argc, char *argv[])
               ShutdownHandler.countdown.Cancel();
            }
 
-        if (!cControl::Control() && !cRecordControls::Active() && !RecordingsHandler.Active() && (Now - cRemote::LastActivity()) > ACTIVITYTIMEOUT) {
+        if (!Control && !cRecordControls::Active() && !RecordingsHandler.Active() && (Now - cRemote::LastActivity()) > ACTIVITYTIMEOUT) {
            // Shutdown:
            // Check whether VDR will be ready for shutdown in SHUTDOWNWAIT seconds:
            time_t Soon = Now + SHUTDOWNWAIT;