[h323plus] Twice H323Connection::Unlock() call issue

Jan Willamowius jan at willamowius.de
Fri Jan 22 13:38:46 EST 2016


Hi Iurii,

I have put a fix similar to what you suggested into the CVS.

Thanks for posting all these bug reports! I'll try to slowly work my
way through them; Simon seems to be busy.

It would help me if you could post your suggested changes as "diff -u"
against the CVS.

Regards,
Jan

-- 
Jan Willamowius, Founder of the GNU Gatekeeper Project
EMail  : jan at willamowius.de
Website: http://www.gnugk.org
Support: http://www.willamowius.com/gnugk-support.html

Relaxed Communications GmbH
Frahmredder 91
22393 Hamburg
Geschäftsführer: Jan Willamowius
HRB 125261 (Amtsgericht Hamburg)
USt-IdNr: DE286003584

Iurii Gordiienko wrote:
> Hi Jan, 
> I think, your solution will work, but in my opinion we have to fix  H323Connection::SendSignalSetup function, because it (only) has strange behavior. It has different internal state (locked and unlocked) for the same return reason (Not H323Connection::EndedByCallerAbort). 
> Take a look to my proposition (a bit draft variant but in anyway) - in worst case (if not able to Lock the mutexes) we only lost the real reason (why does it fails) and return EndedByCallerAbort. 
> H323Connection::SendSignalSetup 
> {       CallEndReason reason = NumCallEndReasons; 
> 
> 
> //... 
>       // Do the transport connect       connectionState = AwaitingTransportConnect; 
>       // Release the mutex as can deadlock trying to clear call during connect.       Unlock(); 
>       PBoolean connectFailed = false;       if (!signallingChannel->IsOpen()) {         signallingChannel->SetWriteTimeout(100);         connectFailed = !signallingChannel->Connect();       } 
>       // See if transport connect failed, abort if so.       if (connectFailed) {         connectionState = NoConnectionActive;         switch (signallingChannel->GetErrorNumber()) {           case ENETUNREACH :             reason = EndedByUnreachable;             break;           case ECONNREFUSED :             reason = EndedByNoEndPoint;             break;           case ETIMEDOUT :             reason =  EndedByHostOffline;             break;           default:              reason = EndedByConnectFail;         }       } 
>   }  
>   // Lock while checking for shutting down.   if (!Lock())     return EndedByCallerAbort;   else if(NumCallEndReasons != reason)     return reason; 
> //... 
>     return reason; } 
> Thanks -- Iurii Gordiienko 
> 
> --- Original message --- 
> From: "Jan Willamowius" <jan at willamowius.de> 
> Date: 27 December 2015, 19:47:21 
> 
> Hi Iurii,
> 
> PTLib 2.10.x and 2.11.x have a method to check, if a mutex is already
> locked, but they changed the API and from 2.12.x on there is no way to
> check except to try to lock it.
> 
> Replace H323Connection::Unlock() in src/h323.cxx with this version:
> 
> void H323Connection::Unlock()
> {
>   // calling Signal() on a mutex that isn't locked is causing an assert,
>   // so we make sure it is locked before the Signal()
>   innerMutex.Wait(0);
>   innerMutex.Signal();
>   outerMutex.Wait(0);
>   outerMutex.Signal();
> }
> 
> Please try the change and report back how it works.
> Then I'll commit the code to the CVS.
> 
> Regards,
> Jan
> 
> -- 
> Jan Willamowius, Founder of the GNU Gatekeeper Project
> EMail  : jan at willamowius.de 
> Website: http://www.gnugk.org 
> Support: http://www.willamowius.com/gnugk-support.html 
> 
> Relaxed Communications GmbH
> Frahmredder 91
> 22393 Hamburg
> Geschäftsführer: Jan Willamowius
> HRB 125261 (Amtsgericht Hamburg)
> USt-IdNr: DE286003584
> 
> 
> Iurii Gordiienko wrote:
> > Hi, 
> > I have found a bug with twice H323Connection::Unlock() call if we have an connection issue. 
> > Take a look: 
> > 1.We are running connection.SendSignalSetup function from H225CallThread::Main() and if it returns any reason!=H323Connection::EndedByCallerAbort we will Unlock the connection. 
> > 
> > void H225CallThread::Main() {   PTRACE(3, "H225\tStarted call thread"); 
> >   if (connection.Lock()) {     H323Connection::CallEndReason reason = connection.SendSignalSetup(alias, address); 
> >     // Special case, if we aborted the call then already will be unlocked     if (reason != H323Connection::EndedByCallerAbort)       connection.Unlock(); 
> >     // bla-bla-bla } 
> > 
> > 2.But we have case when connection.SendSignalSetup returns non H323Connection::EndedByCallerAbort reason but has own unlocked state - for this case PTLIB will unlock the mutex twice with assert (as minimum) message. 
> > H323Connection::SendSignalSetup(const PString & alias, 
> >                                                               const H323TransportAddress & address) { //bla-bla-bla 
> >       // Release the mutex as can deadlock trying to clear call during connect.       Unlock(); 
> >       PBoolean connectFailed = false;       if (!signallingChannel->IsOpen()) {         signallingChannel->SetWriteTimeout(100);         connectFailed = !signallingChannel->Connect();       } 
> >       // See if transport connect failed, abort if so.       if (connectFailed) {         connectionState = NoConnectionActive;         switch (signallingChannel->GetErrorNumber()) {           case ENETUNREACH :             return EndedByUnreachable;           case ECONNREFUSED :             return EndedByNoEndPoint;           case ETIMEDOUT :             return EndedByHostOffline;         }         return EndedByConnectFail;       } 
> > } 
> > Thanks -- Iurii Gordiienko 
> 





More information about the h323plus mailing list