Re: Early releases of lock in mamaSubscription destroy/deallocate logic


Slade, Michael J
 

Hi Frank,

 

Why do we need to unlock the mutex before the deallocate call? I understand that this call destroys the lock, but with the proposed wrapper around the lock the destroy call can defer the actual release of memory to the unlock call.

 

The early release could be exploited because the MamaSubscription Java wrapper calls deallocate in its finalize method. Due to this, the GC thread could acquire the lock and release memory in mamaSubscriptionImpl_deallocate while the lock-releasing thread is trying to use this memory to invoke the user callback.

 

Thanks,

Mike

 

From: Frank Quinn [mailto:fquinn@...]
Sent: 27 April 2020 22:04
To: Slade, Michael J (CIB Tech, GBR) <michael.j.slade@...>
Cc: openmama-dev@...
Subject: RE: [Openmama-dev] Early releases of lock in mamaSubscription destroy/deallocate logic

 

Hi Mike,

 

Could you shed some more light on what exactly the problem is here? I have had a refresher in that area, and from what I can see, I can’t think of any reason to unlock that particular mutex before the user callback so I’m not opposed to moving until after that callback if that will resolve whatever issue you’re seeing? It will need to be before the deallocate though.

 

The only reason I could think of for why this was done in the first place is to avoid deadlock if the user calls something like mamaSubscription_deallocate or something else that uses that mutex from the callback, but then I guess they’d learn pretty quickly if they had fallen foul of that. Then I thought this might be somehow exploited in one of the Java or .NET wrappers but couldn’t find any evidence of that either. Plus that callback is more informative than anything else for the application, so if they are attempting to deallocate, and that’s causing funny business (or something like that?), we just need to make it clear in the callback documentation that they shouldn’t be doing that.

 

Cheers,

Frank

 

Frank Quinn, Cascadium | +44 (0) 28 8678 8015 | http://cascadium.io

 

From: Openmama-dev@... <Openmama-dev@...> On Behalf Of Bill Torpey via lists.openmama.org
Sent: 05 April 2020 15:02
To: Slade, Michael J <michael.j.slade@...>
Cc: openmama-dev@...
Subject: Re: [Openmama-dev] Early releases of lock in mamaSubscription destroy/deallocate logic

 

At first glance, that sounds like just “kicking the can down the road” — i.e., you’re still left with the problem of what to do with these wrappers such that you can tear them down in a thread-safe manner.

 

Having said that, if you have an implementation that works, I’m sure that others would be interested, so maybe put it up on GitHub or something?

 

My personal preference would be to try to find something on the intertubes that has been tested up the wazoo — concurrency is hard, and the hardest part IMO is tear-down of event sources/sinks.

 

On Apr 3, 2020, at 10:39 AM, Slade, Michael J <michael.j.slade@...> wrote:

 

Thank you for your reply Bill.

 

To get around the problem with attempting to destroy the mutex while it is locked, could we instead define our own wrapper around a recursive mutex which handles the following:

1.      Keep a count of how many locks have been performed by current thread.

2.      Defer destroy to unlock call if mutex currently locked.

 

This will keep all destroy/deallocate logic thread safe and stop the need for early unlocks.

 

Mike

 

From: Bill Torpey [mailto:wallstprog@...] 
Sent: 01 April 2020 18:10
To: Slade, Michael J (CIB Tech, GBR) <michael.j.slade@...>
Cc: openmama-dev@...
Subject: Re: [Openmama-dev] Early releases of lock in mamaSubscription destroy/deallocate logic

 

As far as I can tell, there’s no perfect solution here.    From https://linux.die.net/man/3/pthread_mutex_destroy 

 

Attempting to destroy a locked mutex results in undefined behavior.

 

So you either destroy the locked mutex, which is UB, or you unlock it first.  In the second case, another thread could grab it, which would likely cause a crash at some point.  

 

You can make the second problem go away by doing something like queueing the actual destroy in such a way that you know for certain that no other threads are waiting on the mutex, but that’s a fair bit of work, and tricky in its own right.  We took that approach in our application, and it generally works well, and avoids a whole host of lifetime-related problems in OpenMAMA.

 



On Mar 31, 2020, at 11:50 AM, Slade, Michael J via Lists.Openmama.Org <michael.j.slade=jpmorgan.com@...> wrote:

 

Hi OpenMAMA Devs,

 

In the subscription destroy/deallocate logic, there are a couple of functions which seem to release the lock used to protect access to the subscription structure early – i.e. before the user callback ‘mamaSubscriptionImpl_invokeDestroyedCallback’ or ‘mamaSubscriptionImpl_deallocate’ function is invoked. Is anyone able to explain why the lock is released in this way, and not held until the end of the function?
 
Thanks in advance,
Mike

mamaSubscription_destroy:2848

mama_status mamaSubscription_destroy(mamaSubscription subscription)

{

    /* Returns. */

    mama_status ret = MAMA_STATUS_NULL_ARG;

    if(NULL != subscription)

    {

        /* Get the impl. */

        mamaSubscriptionImpl *impl = (mamaSubscriptionImpl *)subscription;

 

        mamaSubscription_deactivate(subscription);

 

         wlock_lock(impl->mCreateDestroyLock);

 

        /* The next action will depend on the current state of the subscription. */

        switch(wInterlocked_read(&impl->mState))

        {

                /* Otherwise the subscription has been correctly removed from the throttle. */

                /* Fall through to perform the remaining clean-up. */

 

                /* For the following states the subscription is not active, simply perform clean-up. */

            case MAMA_SUBSCRIPTION_SETUP:

            case MAMA_SUBSCRIPTION_DEACTIVATED:

                 mamaSubscription_cleanup(subscription);

                 mamaSubscriptionImpl_setState(impl, MAMA_SUBSCRIPTION_DESTROYED);

                  wlock_unlock(impl->mCreateDestroyLock);

                 mamaSubscriptionImpl_invokeDestroyedCallback(subscription);

                 return MAMA_STATUS_OK;

                break;

 

mamaSubscriptionImpl_onSubscriptionDestroyed:3292+3303

 

void MAMACALLTYPE mamaSubscriptionImpl_onSubscriptionDestroyed(mamaSubscription subscription, void *closure)
{
    /* Obtain the impl from the subscription object. */
    mamaSubscriptionImpl *impl = (mamaSubscriptionImpl *)subscription;
    if(NULL != impl)
    {
 
        if(NULL != impl->mQueue)
            mamaQueue_decrementObjectCount(&impl->mLockHandle, impl->mQueue);
 
        /* Lock the mutex. */
        wlock_lock(impl->mCreateDestroyLock);
 
 
        /* The next action will depend on the current state of the subscription. */
        switch(wInterlocked_read(&impl->mState))
        {
                /* The subscription is being deactivated. */
            case MAMA_SUBSCRIPTION_DEACTIVATING:
                /* Change the state. */
                mamaSubscriptionImpl_setState(impl, MAMA_SUBSCRIPTION_DEACTIVATED);
                break;
 
                /* The subscription is being deallocated, i.e. mamaSubscription_deallocate has been called
                 * before the destroy callback has come in from the bridge.
                 */
            case MAMA_SUBSCRIPTION_DEALLOCATING :
                 mamaSubscription_cleanup(subscription);
                 wlock_unlock(impl->mCreateDestroyLock);
                 mamaSubscriptionImpl_invokeDestroyedCallback(impl);
                /* Delete the subscription. */
                mamaSubscriptionImpl_deallocate(impl);
                return;
                break;
 
                /* The subscription is being destroyed. */
            case MAMA_SUBSCRIPTION_DESTROYING :
                 mamaSubscription_cleanup(subscription);
                 mamaSubscriptionImpl_setState(impl, MAMA_SUBSCRIPTION_DESTROYED);
                  wlock_unlock(impl->mCreateDestroyLock);
                 mamaSubscriptionImpl_invokeDestroyedCallback(impl);
                 return;
                break;

This message is confidential and subject to terms at: https://www.jpmorgan.com/emaildisclaimer including on confidential, privileged or legal entity information, viruses and monitoring of electronic messages. If you are not the intended recipient, please delete this message and notify the sender immediately. Any unauthorized use is strictly prohibited.

 

This message is confidential and subject to terms at: https://www.jpmorgan.com/emaildisclaimer including on confidential, privileged or legal entity information, viruses and monitoring of electronic messages. If you are not the intended recipient, please delete this message and notify the sender immediately. Any unauthorized use is strictly prohibited.

 

This message is confidential and subject to terms at: https://www.jpmorgan.com/emaildisclaimer including on confidential, privileged or legal entity information, viruses and monitoring of electronic messages. If you are not the intended recipient, please delete this message and notify the sender immediately. Any unauthorized use is strictly prohibited.

Join Openmama-dev@lists.openmama.org to automatically receive all group messages.