Added some missing locks when calling functions from cStatus or cSkin*

4 files changed, 65 insertions, 33 deletions

diff --git a/HISTORY b/HISTORY
index c8cf54f3..2f4feb33 100644
--- a/HISTORY
+++ b/HISTORY
@@ -9135,3 +9135,6 @@ Video Disk Recorder Revision History
 - Updated links in the INSTALL file (thanks to Chris Mayo).
 - Fixed detecting whether a CAM replies to queries, which didn't work on some systems
   since the implementation of RI_HOST_CONTROL (reported by Daniel Scheller).
+- Added some missing locks when calling functions from cStatus or cSkin*, and added
+  some text to status.h and skins.h, explaining the locking situation when such
+  functions are called.
diff --git a/menu.c b/menu.c
index 8f83ae00..209482a6 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.38 2017/06/20 15:02:39 kls Exp $
+ * $Id: menu.c 4.39 2017/06/21 09:19:59 kls Exp $
  */
 
 #include "menu.h"
@@ -1714,7 +1714,10 @@ eOSState cMenuWhatsOn::ProcessKey(eKeys Key)
                         for (cOsdItem *item = First(); item; item = Next(item)) {
                             if (((cMenuScheduleItem *)item)->channel->Number() == cDevice::CurrentChannel()) {
                                SetCurrent(item);
-                               Display();
+                               {
+                                 LOCK_SCHEDULES_READ;
+                                 Display();
+                               }
                                LOCK_CHANNELS_READ;
                                SetHelpKeys(Channels);
                                break;
@@ -1733,8 +1736,10 @@ eOSState cMenuWhatsOn::ProcessKey(eKeys Key)
        }
      }
   else if (!HasSubMenu()) {
-     if (HadSubMenu && Update())
+     if (HadSubMenu && Update()) {
+        LOCK_SCHEDULES_READ;
         Display();
+        }
      if (Key != kNone) {
         LOCK_CHANNELS_READ;
         SetHelpKeys(Channels);
@@ -1953,32 +1958,30 @@ eOSState cMenuSchedule::Number(void)
 eOSState cMenuSchedule::Record(void)
 {
   if (cMenuScheduleItem *item = (cMenuScheduleItem *)Get(Current())) {
-     {
-       LOCK_TIMERS_WRITE;
-       LOCK_CHANNELS_READ;
-       LOCK_SCHEDULES_READ;
-       Timers->SetExplicitModify();
-       if (item->timerMatch == tmFull) {
-          if (cTimer *Timer = Timers->GetMatch(item->event))
-             return AddSubMenu(new cMenuEditTimer(Timer));
-          }
-       cTimer *Timer = new cTimer(item->event);
-       if (Setup.SVDRPPeering && *Setup.SVDRPDefaultHost)
-          Timer->SetRemote(Setup.SVDRPDefaultHost);
-       if (cTimer *t = Timers->GetTimer(Timer)) {
-          delete Timer;
-          Timer = t;
-          return AddSubMenu(new cMenuEditTimer(Timer));
-          }
-       if (Timer->Matches(0, false, NEWTIMERLIMIT))
-          return AddSubMenu(new cMenuEditTimer(Timer, true));
-       Timers->Add(Timer);
-       Timers->SetModified();
-       if (!HandleRemoteModifications(Timer)) {
-          // must add the timer before HandleRemoteModifications to get proper log messages with timer ids
-          Timers->Del(Timer);
-          }
-     }
+     LOCK_TIMERS_WRITE;
+     LOCK_CHANNELS_READ;
+     LOCK_SCHEDULES_READ;
+     Timers->SetExplicitModify();
+     if (item->timerMatch == tmFull) {
+        if (cTimer *Timer = Timers->GetMatch(item->event))
+           return AddSubMenu(new cMenuEditTimer(Timer));
+        }
+     cTimer *Timer = new cTimer(item->event);
+     if (Setup.SVDRPPeering && *Setup.SVDRPDefaultHost)
+        Timer->SetRemote(Setup.SVDRPDefaultHost);
+     if (cTimer *t = Timers->GetTimer(Timer)) {
+        delete Timer;
+        Timer = t;
+        return AddSubMenu(new cMenuEditTimer(Timer));
+        }
+     if (Timer->Matches(0, false, NEWTIMERLIMIT))
+        return AddSubMenu(new cMenuEditTimer(Timer, true));
+     Timers->Add(Timer);
+     Timers->SetModified();
+     if (!HandleRemoteModifications(Timer)) {
+        // must add the timer before HandleRemoteModifications to get proper log messages with timer ids
+        Timers->Del(Timer);
+        }
      if (HasSubMenu())
         CloseSubMenu();
      if (Update())
@@ -2077,8 +2080,10 @@ eOSState cMenuSchedule::ProcessKey(eKeys Key)
            Set(Timers, Channels, Channel, true);
            }
         }
-     else if (HadSubMenu && Update())
+     else if (HadSubMenu && Update()) {
+        LOCK_SCHEDULES_READ;
         Display();
+        }
      if (Key != kNone)
         SetHelpKeys();
      }
@@ -3189,8 +3194,10 @@ eOSState cMenuRecordings::ProcessKey(eKeys Key)
         return state; // closes all recording menus except for the top one
      Set(); // this is the top level menu, so we refresh it...
      Open(true); // ...and open any necessary submenus to show the new name
-     if (!HasSubMenu())
+     if (!HasSubMenu()) {
+        LOCK_RECORDINGS_READ;
         Display();
+        }
      path = NULL;
      fileName = NULL;
      }
diff --git a/skins.h b/skins.h
index 2286087b..f994d47d 100644
--- a/skins.h
+++ b/skins.h
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: skins.h 4.1 2015/09/10 11:19:48 kls Exp $
+ * $Id: skins.h 4.2 2017/06/21 09:40:39 kls Exp $
  */
 
 #ifndef __SKINS_H
@@ -21,6 +21,17 @@
 #include "timers.h"
 #include "tools.h"
 
+// Several member functions of the following classes are called with a pointer to
+// an object from a global list (cTimer, cChannel, cRecording or cEvent). In these
+// cases the core VDR code holds a lock on the respective list. The called function
+// may itself set a read lock (not a write lock!) on this list, because read locks
+// can be nested. It may also set read locks (not write locks!) on higher order lists.
+// For instance, a function that is called with a cChannel may lock cRecordings and/or
+// cSchedules (which contains cEvent objects), but not cTimers. If a plugin needs to
+// set locks of its own (on mutexes defined inside the plugin code), it shall do so
+// after setting any locks on VDR's global lists, and it shall always set these
+// locks in the same sequence, to avoid deadlocks.
+
 enum eMessageType { mtStatus = 0, mtInfo, mtWarning, mtError }; // will be used to calculate color offsets!
 
 class cSkinDisplay {
diff --git a/status.h b/status.h
index 6d1b9df5..32a9c2ad 100644
--- a/status.h
+++ b/status.h
@@ -4,7 +4,7 @@
  * See the main source file 'vdr.c' for copyright information and
  * how to reach the author.
  *
- * $Id: status.h 4.1 2015/08/02 10:34:44 kls Exp $
+ * $Id: status.h 4.2 2017/06/21 09:40:20 kls Exp $
  */
 
 #ifndef __STATUS_H
@@ -15,6 +15,17 @@
 #include "player.h"
 #include "tools.h"
 
+// Several member functions of the following classes are called with a pointer to
+// an object from a global list (cTimer, cChannel, cRecording or cEvent). In these
+// cases the core VDR code holds a lock on the respective list. The called function
+// may itself set a read lock (not a write lock!) on this list, because read locks
+// can be nested. It may also set read locks (not write locks!) on higher order lists.
+// For instance, a function that is called with a cChannel may lock cRecordings and/or
+// cSchedules (which contains cEvent objects), but not cTimers. If a plugin needs to
+// set locks of its own (on mutexes defined inside the plugin code), it shall do so
+// after setting any locks on VDR's global lists, and it shall always set these
+// locks in the same sequence, to avoid deadlocks.
+
 enum eTimerChange { tcMod, tcAdd, tcDel }; // tcMod is obsolete and no longer used!
 
 class cTimer;