incorrect ref counting for timer queues

Reported by mdavidsaver on 2011-05-23
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
EPICS Base
Undecided
Unassigned

Bug Description

Shared timer queues are unconditionally deleted by the first call to timerQueueActiveMgr::release. See attached test case. Patch will follow.

Test will crash on "T1->destroy();" because the queue is free'd by "Q2->release();".

Related branches

mdavidsaver (mdavidsaver) wrote :
Jeff Hill (johill-lanl) wrote :

It appears that this bug was introduced by the fix for mantis 332 on 2009-2-10

Perhaps we need a fix like this:

before:

void timerQueueActiveMgr ::
    release ( epicsTimerQueueActiveForC & queue )
{
    timerQueueActiveMgrPrivate * pPriv = & queue;
    {
        Guard locker ( this->mutex );
        assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
        queue.timerQueueActiveMgrPrivate::referenceCount--;
        if ( queue.timerQueueActiveMgrPrivate::referenceCount == 0u ) {
            if ( queue.sharingOK () ) {
                this->sharedQueueList.remove ( queue );
            }
        }
    }
    // delete only after we release the guard in case the embedded
    // reference is the last one and this object is destroyed
    // as a side effect
    delete pPriv;
}

after:

void timerQueueActiveMgr ::
    release ( epicsTimerQueueActiveForC & queue )
{
    timerQueueActiveMgrPrivate * pPriv = & queue;
    {
        Guard locker ( this->mutex );
        assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
        queue.timerQueueActiveMgrPrivate::referenceCount--;
        if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
            pPriv = 0;
        }
        else if ( queue.sharingOK () ) {
            this->sharedQueueList.remove ( queue );
        }
    }
    // delete only after we release the guard in case the embedded
    // reference is the last one and this object is destroyed
    // as a side effect
    idelete pPriv;
}

Hi Jeff,

I think this would still delete the queue before the count is zero. Try
putting:

assert ( queue.timerQueueActiveMgrPrivate::referenceCount == 0u );

before the delete. I think that fundumentally the delete must be in a
conditional, or a second return path is needed.

On 05/24/11 20:10, Jeff Hill wrote:
> It appears that this bug was introduced by the fix for mantis 332 on
> 2009-2-10
>
> Perhaps we need a fix like this:

...

> after:
>
> void timerQueueActiveMgr ::
> release ( epicsTimerQueueActiveForC & queue )
> {
> timerQueueActiveMgrPrivate * pPriv = & queue;
> {
> Guard locker ( this->mutex );
> assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
> queue.timerQueueActiveMgrPrivate::referenceCount--;
> if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
> pPriv = 0;
> }
> else if ( queue.sharingOK () ) {
> this->sharedQueueList.remove ( queue );
> }
> }
> // delete only after we release the guard in case the embedded
> // reference is the last one and this object is destroyed
> // as a side effect
> idelete pPriv;
> }
>

Jeff Hill (johill-lanl) wrote :

Hi David,

> I think this would still delete the queue before the count is zero

Note that there is the if statement which sets pPriv to nill, and of course the delete will not fire if pPriv is nill

        if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
            pPriv = 0;
        }

> I think that fundamentally the delete must be in a conditional, or a second return path is needed.

Note that the delete can not occur when the guard is still active. So we determine if the delete will need to occur when we have the lock, and then defer it until after the guard has been released

mdavidsaver (mdavidsaver) wrote :

On 5/26/2011 10:23 AM, Jeff Hill wrote:
> Hi David,
>
>> I think this would still delete the queue before the count is zero
> Note that there is the if statement which sets pPriv to nill, and of
> course the delete will not fire if pPriv is nill
>
> if ( queue.timerQueueActiveMgrPrivate::referenceCount> 0u ) {
> pPriv = 0;
> }

Ok, I see. I would suggest replacing "pPriv = 0;" with "return;" since
I think this makes it more obvious what is going on...

>> I think that fundamentally the delete must be in a conditional, or a
> second return path is needed.
>
> Note that the delete can not occur when the guard is still active. So we
> determine if the delete will need to occur when we have the lock, and
> then defer it until after the guard has been released
>

Jeff Hill (johill-lanl) wrote :

ok with that mod

I built here with this version and epicsTimerTest passes, but my version does not have your new test

   {
        Guard locker ( this->mutex );
        assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
        queue.timerQueueActiveMgrPrivate::referenceCount--;
        if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
            return;
        }
        else if ( queue.sharingOK () ) {
            this->sharedQueueList.remove ( queue );
        }
    }
    // delete only after we release the guard in case the embedded
    // reference is the last one and this object is destroyed
    // as a side effect
    timerQueueActiveMgrPrivate * const pPriv = & queue;
    delete pPriv;

Jeff Hill (johill-lanl) wrote :

>bzr diff timerQueueActiveMgr.cpp
=== modified file 'src/libCom/timer/timerQueueActiveMgr.cpp'
--- src/libCom/timer/timerQueueActiveMgr.cpp 2011-01-15 01:59:34 +0000
+++ src/libCom/timer/timerQueueActiveMgr.cpp 2011-05-26 21:24:51 +0000
@@ -57,20 +57,21 @@
 void timerQueueActiveMgr ::
     release ( epicsTimerQueueActiveForC & queue )
 {
- timerQueueActiveMgrPrivate * pPriv = & queue;
     {
         Guard locker ( this->mutex );
         assert ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u );
         queue.timerQueueActiveMgrPrivate::referenceCount--;
- if ( queue.timerQueueActiveMgrPrivate::referenceCount == 0u ) {
- if ( queue.sharingOK () ) {
- this->sharedQueueList.remove ( queue );
- }
+ if ( queue.timerQueueActiveMgrPrivate::referenceCount > 0u ) {
+ return;
+ }
+ else if ( queue.sharingOK () ) {
+ this->sharedQueueList.remove ( queue );
         }
     }
- // delete only after we release the guard in case the embedded
+ // delete only after we release the guard in case the embedded
     // reference is the last one and this object is destroyed
     // as a side effect
+ timerQueueActiveMgrPrivate * const pPriv = & queue;
     delete pPriv;
 }

mdavidsaver (mdavidsaver) wrote :

Ok, looks like this works. I'm attaching an updated patch against the
3.14 branch.

Andrew Johnson (anj) on 2011-06-01
Changed in epics-base:
status: New → Fix Committed
Andrew Johnson (anj) on 2011-12-12
Changed in epics-base:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers