Fix CCheckQueue IsIdle (potential) race condition #9495

pull JeremyRubin wants to merge 1 commits into bitcoin:master from JeremyRubin:checkqueue-control-lock changing 1 files +12 −10

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:None 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:None 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:None 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:None 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

Contributors

Linked (view graph)

#9497 CCheckQueue Unit Tests #9938 Lock-Free CheckQueue