Fix CCheckQueue IsIdle (potential) race condition

JeremyRubin commented at 4:23 pm on January 9, 2017: contributor

There’s a small race condition in the CCheckQueue code which I don’t think is currently an active issue, but future code might break.

IsIdle is not threadsafe. If two concurrent CCheckqueueControls are made, they could simultaneously report being idle, and fail to panic.

Furthermore, in the case a concurrent CCheckqueueControl is made, most likely waiting until control is relinquished is the right behavior rather than failing an assert.

in src/checkqueue.h: in 958ee1d3d8 outdated

181@@ -185,8 +182,7 @@ class CCheckQueueControl
182     {
183         // passed queue is supposed to be unused, or NULL
184         if (pqueue != NULL) {
185-            bool isIdle = pqueue->IsIdle();
186-            assert(isIdle);
187+            ENTER_CRITICAL_SECTION(pqueue->ControlMutex);

sipa commented at 4:49 pm on January 9, 2017:

Can you do this as a unique_lock field inside CCheckQueueControl (so we don’t need to rely on an explicit destructor to be correct)?

JeremyRubin commented at 4:56 pm on January 9, 2017:

Unfortunately this is the “easiest” way to take care of this given that we want debug order & the CMutex class has no default constructor/moveable semantics. Certainly can be fixed to RAII easily later.

sipa commented at 4:59 pm on January 9, 2017:

Understood. Let’s improve that in a separate PR later.

ryanofsky approved

ryanofsky commented at 8:33 pm on January 11, 2017: member

utACK 958ee1d3d844d1f2f07b271ad7ed9bb117e5a766 with one nit:

Would suggest deleting the CCheckQueueControl default copy constructor and copy operator and making the pqueue member const, to give more assurance that the ENTER/LEAVE calls will always be paired.

in src/checkqueue.h: in 2a58f73601 outdated

183-    {
184-        // passed queue is supposed to be unused, or NULL
185-        if (pqueue != NULL) {
186-            ENTER_CRITICAL_SECTION(pqueue->ControlMutex);
187-        }
188+	CCheckQueueControl<T>() = delete;

ryanofsky commented at 11:01 pm on January 11, 2017:

Nit: I think you can drop the <T>

ryanofsky approved

ryanofsky commented at 11:01 pm on January 11, 2017: member

utACK 2a58f73601ad36d0531fe9e69e1affd9229d8b96

JeremyRubin force-pushed on Jan 20, 2017

JeremyRubin commented at 10:14 pm on January 20, 2017: contributor

Squashed and addressed @ryanofsky’s nit about template args in constructors.

in src/checkqueue.h: in 020acd8f81 outdated

189+	CCheckQueueControl() = delete;
190+	CCheckQueueControl(const CCheckQueueControl&) = delete;
191+	CCheckQueueControl& operator=(const CCheckQueueControl&) = delete;
192+	explicit CCheckQueueControl(CCheckQueue<T> * const pqueueIn) : pqueue(pqueueIn), fDone(false)
193+	{
194+		// passed queue is supposed to be unused, or NULL

sipa commented at 10:52 pm on January 20, 2017:

Weird indendation.

JeremyRubin commented at 11:16 pm on January 20, 2017:

Oops – somehow, my expandtab and tabstop=4 got unset.

sipa commented at 10:53 pm on January 20, 2017: member

utACK

Fix CCheckQueue IsIdle (potential) race condition and remove dangerous constructors. f5daff7307

JeremyRubin force-pushed on Jan 20, 2017

JeremyRubin commented at 11:26 pm on January 20, 2017: contributor

sorry for causing extra review – pushed and squashed indention fix.

in src/checkqueue.h: in f5daff7307

126@@ -127,6 +127,9 @@ class CCheckQueue
127     }
128 
129 public:
130+    //! Mutex to ensure only one concurrent CCheckQueueControl
131+    boost::mutex ControlMutex;

kallewoof commented at 9:48 pm on February 28, 2017:

Nit: Is there some coding standard for mutexes which makes you capitalize this ivar? If not, maybe controlMutex would be more consistent.

kallewoof commented at 9:49 pm on February 28, 2017: member

utACK f5daff7

JeremyRubin commented at 3:34 pm on March 27, 2017: contributor

#9497 was merged, which included this. Closing.

JeremyRubin closed this on Mar 27, 2017

DrahtBot locked this on Sep 8, 2021

Fix CCheckQueue IsIdle (potential) race condition #9495