Re: C++ Broker: segfault in TopicExchange::isBound()

[Gordon Sim <gs...@redhat.com>]

rspringer@mail.etinternational.com wrote:
> All,
> We've been using Qpid M4 and 0.5 internally, both with C++, and have been
> seeing occasional segfaults within the broker.  They all seem to occur
> inside TopicExchange::isBound() (the one with 3 parameters, not 2).
> 
> Looking at the access patterns in this function, and judging by the
> core dumps, it appears that multiple threads may access and modify
> the class member "bindings" simultaneously, which seems like it
> has the potential to cause a segfault.

Thanks very much for the report! I think you are exactly right, the 
topic exchange has no locking on the isBound() check and is therefore 
unsafe.

I raised a JIRA: https://issues.apache.org/jira/browse/QPID-1963

> Since this is a race condition (if we're right), it's hard to
> say for sure that the problem has been resolved, but since doing
> the below, we've not had a segfault (roughly 5 hours so far,
> rather than every 2-3 hours or so).
>  - We added locks to all accesses of bindings throughout the
>    file.  I believe the new ones were limited to isBound.
>  - This may not be necessary at all, but to be safe (while
>    we were investigating the problem), we changed all lock
>    types from RWlock::ScopedLocks to Mutex::ScopedLocks, to avoid
>    being tripped up by pthread (read/write) mutex semantics.

My suggested fix would be to simply hold a read lock across the 
isBound() method (as per attached patch). I have a reproducer that 
causes a crash within a few minutes and will try this fix out with it 
(so far so good).

> We wanted to get the opinion of the experienced Qpid developers
> on this list before opening a bug, but again, so far it seems to
> be much more stable (we're hammering on Session::exchangeBound()
> in our testing, which may be why we're seeing it in the first place).
> 
> I'm not sure if the other exchange types have the same problem or not.

 From a brief examination, the other types do appear to have locking for 
that method. I'll run my test against them as well however.

> Thanks!
> -Rob

Thanks again for raising the issue and for all the investigation you 
have done on this so far!

--Gordon.