You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2016/07/18 16:04:53 UTC
Review Request 50142: Bug 1333767: Backup crash during re-connect in
failover
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50142/
-----------------------------------------------------------
Review request for qpid, Gordon Sim and Ted Ross.
Repository: qpid-cpp
Description
-------
Fixed bug in backup recovery consistent with stack traces from a customer and
similar traces reproduced as explained in
https://bugzilla.redhat.com/show_bug.cgi?id=1333767#c19.
The bug was in BrokerReplicator: an interrupted failover left the UpdateTracker
in place with a queue list. The next failover created a new UpdateTracker and
deleted the old one, which triggered deletion of queues. This caused re-entrant
deletion of bridges while Link::ioThreadProcessing is iterating over the bridge
list, which could cause the iteration to go past the end of the list and access
invalid memory.
The exact circumstances are hard to reproduce, it requires multiple steps:
1. Backup receives queue creation events and calls to Link::add() with new
bridges, but is disconnected from primary before the Link::ioThreadProcessing()
event. This puts the new bridges on the Link::created list *before* the
BrokerReplicator bridge.
2. Backup re-connects and creates an UpdateTracker with a list of all queues.
Before the update is complete, the backup is disconnected leaving the
UpdateTracker in place.
3. Backup re-connects again and deletes the old UpdateTracker, which deletes all
the queues and their bridges in the Link::created vector. This means the iterator
in Link::ioThreadProcessing now points after the end of the vector.
Step 1 is needed because normally the BrokerReplicator is the first bridge in
the list, and deleting queues after it leaves the iterator inside bounds. There
needs to be at least one deleted queue before the BrokerReplicator in the list.
There are a number of reasons a failover could be interrupted:
- two primaries are killed in quick succession.
- primary disconnects an expected "ready" backup at the ha-backup-timeout.
- a backup misses a heartbeat during failover.
Diffs
-----
src/qpid/ha/BrokerReplicator.cpp d664f13893a075dae459fbc8d50bbb44751832ba
Diff: https://reviews.apache.org/r/50142/diff/
Testing
-------
Passes qpid ctest suite. No auto regression test, not able to reliabily reproduce. Long run stress tests have not shown the bug since the patch.
Thanks,
Alan Conway
Re: Review Request 50142: Bug 1333767: Backup crash during re-connect
in failover
Posted by Ted Ross <tr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50142/#review142581
-----------------------------------------------------------
Ship it!
Ship It!
- Ted Ross
On July 18, 2016, 12:04 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50142/
> -----------------------------------------------------------
>
> (Updated July 18, 2016, 12:04 p.m.)
>
>
> Review request for qpid, Gordon Sim and Ted Ross.
>
>
> Repository: qpid-cpp
>
>
> Description
> -------
>
> Fixed bug in backup recovery consistent with stack traces from a customer and
> similar traces reproduced as explained in
> https://bugzilla.redhat.com/show_bug.cgi?id=1333767#c19.
>
> The bug was in BrokerReplicator: an interrupted failover left the UpdateTracker
> in place with a queue list. The next failover created a new UpdateTracker and
> deleted the old one, which triggered deletion of queues. This caused re-entrant
> deletion of bridges while Link::ioThreadProcessing is iterating over the bridge
> list, which could cause the iteration to go past the end of the list and access
> invalid memory.
>
> The exact circumstances are hard to reproduce, it requires multiple steps:
>
> 1. Backup receives queue creation events and calls to Link::add() with new
> bridges, but is disconnected from primary before the Link::ioThreadProcessing()
> event. This puts the new bridges on the Link::created list *before* the
> BrokerReplicator bridge.
>
> 2. Backup re-connects and creates an UpdateTracker with a list of all queues.
> Before the update is complete, the backup is disconnected leaving the
> UpdateTracker in place.
>
> 3. Backup re-connects again and deletes the old UpdateTracker, which deletes all
> the queues and their bridges in the Link::created vector. This means the iterator
> in Link::ioThreadProcessing now points after the end of the vector.
>
> Step 1 is needed because normally the BrokerReplicator is the first bridge in
> the list, and deleting queues after it leaves the iterator inside bounds. There
> needs to be at least one deleted queue before the BrokerReplicator in the list.
>
> There are a number of reasons a failover could be interrupted:
>
> - two primaries are killed in quick succession.
> - primary disconnects an expected "ready" backup at the ha-backup-timeout.
> - a backup misses a heartbeat during failover.
>
>
> Diffs
> -----
>
> src/qpid/ha/BrokerReplicator.cpp d664f13893a075dae459fbc8d50bbb44751832ba
>
> Diff: https://reviews.apache.org/r/50142/diff/
>
>
> Testing
> -------
>
> Passes qpid ctest suite. No auto regression test, not able to reliabily reproduce. Long run stress tests have not shown the bug since the patch.
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 50142: Bug 1333767: Backup crash during re-connect
in failover
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50142/#review142571
-----------------------------------------------------------
Ship it!
Ship It!
- Gordon Sim
On July 18, 2016, 4:04 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50142/
> -----------------------------------------------------------
>
> (Updated July 18, 2016, 4:04 p.m.)
>
>
> Review request for qpid, Gordon Sim and Ted Ross.
>
>
> Repository: qpid-cpp
>
>
> Description
> -------
>
> Fixed bug in backup recovery consistent with stack traces from a customer and
> similar traces reproduced as explained in
> https://bugzilla.redhat.com/show_bug.cgi?id=1333767#c19.
>
> The bug was in BrokerReplicator: an interrupted failover left the UpdateTracker
> in place with a queue list. The next failover created a new UpdateTracker and
> deleted the old one, which triggered deletion of queues. This caused re-entrant
> deletion of bridges while Link::ioThreadProcessing is iterating over the bridge
> list, which could cause the iteration to go past the end of the list and access
> invalid memory.
>
> The exact circumstances are hard to reproduce, it requires multiple steps:
>
> 1. Backup receives queue creation events and calls to Link::add() with new
> bridges, but is disconnected from primary before the Link::ioThreadProcessing()
> event. This puts the new bridges on the Link::created list *before* the
> BrokerReplicator bridge.
>
> 2. Backup re-connects and creates an UpdateTracker with a list of all queues.
> Before the update is complete, the backup is disconnected leaving the
> UpdateTracker in place.
>
> 3. Backup re-connects again and deletes the old UpdateTracker, which deletes all
> the queues and their bridges in the Link::created vector. This means the iterator
> in Link::ioThreadProcessing now points after the end of the vector.
>
> Step 1 is needed because normally the BrokerReplicator is the first bridge in
> the list, and deleting queues after it leaves the iterator inside bounds. There
> needs to be at least one deleted queue before the BrokerReplicator in the list.
>
> There are a number of reasons a failover could be interrupted:
>
> - two primaries are killed in quick succession.
> - primary disconnects an expected "ready" backup at the ha-backup-timeout.
> - a backup misses a heartbeat during failover.
>
>
> Diffs
> -----
>
> src/qpid/ha/BrokerReplicator.cpp d664f13893a075dae459fbc8d50bbb44751832ba
>
> Diff: https://reviews.apache.org/r/50142/diff/
>
>
> Testing
> -------
>
> Passes qpid ctest suite. No auto regression test, not able to reliabily reproduce. Long run stress tests have not shown the bug since the patch.
>
>
> Thanks,
>
> Alan Conway
>
>