Fixed lock order in several files

- It is still not finished, but most of the locks should be in the right order now. I am still waiting for a VDR patch to check the lock order dynamically.
a-mair · May 25, 2017 · 3e0774c · 3e0774c
1 parent 6c7dc2d
commit 3e0774c
Show file tree

Hide file tree

Showing 9 changed files with 33 additions and 27 deletions.
diff --git a/pages/channels_widget.ecpp b/pages/channels_widget.ecpp
@@ -25,12 +25,11 @@ if (!logged_in && LiveSetup().UseAuth()) return reply.redirect("login.html");
 <%cpp>
 #if VDRVERSNUM >= 20301
 	LOCK_CHANNELS_READ;
-	if (false)
 #else
 	ReadLock channelsLock( Channels );
 	if ( !channelsLock )
-#endif
 		throw HtmlError( tr("Couldn't aquire access to channels, please try again later.") );
+#endif
 </%cpp>
 <select name="<$ name $>" id="<$ name $>" <{ reply.out() << ( !onchange.empty() ? "onchange=\""+onchange+"\"" : "" ); }>>
 %#if VDRVERSNUM >= 20301

diff --git a/pages/epginfo.ecpp b/pages/epginfo.ecpp
@@ -72,6 +72,9 @@ using namespace std;
 		// check for event:
 		else if (epgid.compare(0, event.length(), event) == 0) {
 #if VDRVERSNUM >= 20301
+			/* Need to lock here channels also, because CreateEpgInfo will lock
+			 * it also and this will result in a wrong lock order */
+			LOCK_CHANNELS_READ;
 			LOCK_SCHEDULES_READ;
 			epgEvent = EpgEvents::CreateEpgInfo(epgid, Schedules);
 #else

diff --git a/pages/errors.vim b/pages/errors.vim
@@ -0,0 +1,3 @@
+make: menu.ecpp: Command not found
+make: channels_widget.ecpp: Command not found
+make: *** [channels_widget.cpp] Error 127
diff --git a/pages/ibox.ecpp b/pages/ibox.ecpp
@@ -46,6 +46,7 @@ TimerConflictNotifier timerNotifier();
 	if (NowReplaying) {
 		RecordingsManagerPtr recManager = LiveRecordingsManager();
 #if VDRVERSNUM >= 20301
+		// is is OK to lock here, because CreateEpgInfo will *not* lock other lists
 		LOCK_RECORDINGS_READ;
 		cRecording *recording = (cRecording *)Recordings->GetByName(NowReplaying);
 #else

diff --git a/pages/recordings.ecpp b/pages/recordings.ecpp
@@ -147,8 +147,12 @@ if (!deleteResult.empty()) {
 			<div class="boxheader"><div><div><$ string(tr("List of recordings")) + " (" + diskinfo + ")" $></div></div></div>
 <%cpp>
 #if VDRVERSNUM >= 20301
-			LOCK_RECORDINGS_READ;
-			if (Recordings->Count() == 0) { // Access VDRs global cRecordings Recordings instance.
+			int rec_cnt;
+			{
+				LOCK_RECORDINGS_READ;
+				rec_cnt = Recordings->Count();  // Access VDRs global cRecordings Recordings instance.
+			}
+			if (rec_cnt == 0) {
 #else
 			if (Recordings.Count() == 0) { // Access VDRs global cRecordings Recordings instance.
 #endif

diff --git a/pages/schedule.ecpp b/pages/schedule.ecpp
@@ -26,11 +26,8 @@ if (!logged_in && LiveSetup().UseAuth()) return reply.redirect("login.html");
 	pageTitle = trVDR("Schedule");
 
 #if VDRVERSNUM >= 20301
-        /* JJJ: Order wrong! Better get first the channel and with unlocked
-         *      channels lock read the schedules
-         */
-        LOCK_SCHEDULES_READ;
-        LOCK_CHANNELS_READ;
+	// we lock here only the channels on purpose
+	LOCK_CHANNELS_READ;
 #else
 	cSchedulesLock schedulesLock;
 	cSchedules const* schedules = cSchedules::Schedules( schedulesLock );
@@ -68,6 +65,7 @@ if (!logged_in && LiveSetup().UseAuth()) return reply.redirect("login.html");
 		throw HtmlError( tr("Couldn't find channel or no channels available. Maybe you mistyped your request?") );
 
 #if VDRVERSNUM >= 20301
+	LOCK_SCHEDULES_READ;
 	cSchedule const* Schedule = Schedules->GetSchedule( (const cChannel *)Channel );
 #else
 	cSchedule const* Schedule = schedules->GetSchedule( Channel );

diff --git a/pages/searchresults.ecpp b/pages/searchresults.ecpp
@@ -51,9 +51,9 @@ bool logged_in(false);
 		<& pageelems.logo &>
 		<& menu active=("searchepg") &>
 		<div class="inhalt">
-%           if (results.size() == 0) {
-                <$ tr("No search results") $>
-%           }
+%			if (results.size() == 0) {
+				<$ tr("No search results") $>
+%			}
 			<table class="listing" cellspacing="0" cellpadding="0">
 <%cpp>
 				string current_day = "";
@@ -63,9 +63,9 @@ bool logged_in(false);
 #endif
 				for (SearchResults::iterator result = results.begin(); result != results.end(); ++result) {
 #if VDRVERSNUM >= 20301
-				        cChannel* channel = (cChannel *)Channels->GetByChannelID(result->Channel());
+					cChannel* channel = (cChannel *)Channels->GetByChannelID(result->Channel());
 #else
-				        cChannel* channel = Channels.GetByChannelID(result->Channel());
+					cChannel* channel = Channels.GetByChannelID(result->Channel());
 #endif
 					if (!channel) continue;
 					string channelname = channel->Name();
@@ -99,7 +99,7 @@ bool logged_in(false);
 					</tr>
 %						current_day = day;
 %					}
-                    <tr>
+					<tr>
 						<td class="action leftcol <? bottom ? "bottomrow"?>"><& pageelems.event_timer epgid=(epgid) &></td>
 						<td class="topaligned <? bottom ? "bottomrow"?>"><div class="withmargin"><a href="schedule.html?channel=<$ channelnr $>"><$ channelname $></a></div></td>
 						<td class="topaligned <? bottom ? "bottomrow"?>"><div class="withmargin nowrap"><$ start $> - <$ end $></div></td>

diff --git a/pages/timerconflicts.ecpp b/pages/timerconflicts.ecpp
@@ -44,11 +44,6 @@ if (!logged_in && LiveSetup().UseAuth()) return reply.redirect("login.html");
 %			} else {
 			<table class="listing" cellspacing="0" cellpadding="0">
 <%cpp>
-#if VDRVERSNUM >= 20301
-				/* JJJ: Lock this later when it is realy used */
-				LOCK_TIMERS_READ;
-				LOCK_SCHEDULES_READ;
-#endif
 				for (TimerConflicts::iterator conflict = timerConflicts.begin(); conflict != timerConflicts.end(); ++conflict) {
 					const std::list< TimerInConflict >& conflTimers = conflict->ConflictingTimers();
 					for (std::list< TimerInConflict >::const_iterator confltimer = conflTimers.begin(); confltimer != conflTimers.end(); ++confltimer) {
@@ -73,6 +68,7 @@ if (!logged_in && LiveSetup().UseAuth()) return reply.redirect("login.html");
 <%cpp>
 						for (std::list<int>::const_iterator timerIndex = confltimer->concurrentTimerIndices.begin(); timerIndex != confltimer->concurrentTimerIndices.end(); ++timerIndex) {
 #if VDRVERSNUM >= 20301
+							LOCK_TIMERS_READ;
 							cTimer* timer = (cTimer *)Timers->Get(*timerIndex-1);
 #else
 							cTimer* timer = Timers.Get(*timerIndex-1);
@@ -94,11 +90,14 @@ if (!logged_in && LiveSetup().UseAuth()) return reply.redirect("login.html");
 								timerStateHint = tr("Timer is active.");
 							}
 
-						        EpgInfoPtr epgEvent;
+							EpgInfoPtr epgEvent;
 							string longDescription;
 							string title;
 #if VDRVERSNUM >= 20301
-							if (!timer->Event()) timer->SetEventFromSchedule(Schedules);
+							if (!timer->Event()) {
+								LOCK_SCHEDULES_READ;
+								timer->SetEventFromSchedule(Schedules);
+							}
 #else
 							if (!timer->Event()) timer->SetEventFromSchedule();
 #endif

diff --git a/pages/timers.ecpp b/pages/timers.ecpp
@@ -75,15 +75,14 @@ static const size_t maximumDescriptionLength = 300;
 			<table class="listing" cellspacing="0" cellpadding="0">
 <%cpp>
 				// output of the timer list:
-#if VDRVERSNUM >= 20301
-				/* JJJ: Lock this inside the loop */
-				LOCK_SCHEDULES_READ;
-#endif
 				for (SortedTimers::iterator timer = timers.begin(); timer != timers.end(); ++timer) {
 				    EpgInfoPtr epgEvent;
 					string longDescription;
 #if VDRVERSNUM >= 20301
-					if (!timer->Event()) timer->SetEventFromSchedule(Schedules);
+					if (!timer->Event()) {
+						LOCK_SCHEDULES_READ;
+						timer->SetEventFromSchedule(Schedules);
+					}
 #else
 					if (!timer->Event()) timer->SetEventFromSchedule();
 #endif