Qpid
  1. Qpid
  2. QPID-2320

Failed acquire on LVQ causes broker crash

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 0.6
    • Fix Version/s: 0.6, 0.7
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      If an acquire fails for an LVQ because the message has already been acquired by some other subscriber, the failed attempt causes the broker to crash. This is due to lack of proper bounds checking in Queue::acquire() for the LVQ case and was I believe introduced by http://svn.apache.org/viewvc?view=revision&revision=834172.

        Activity

        Gordon Sim created issue -
        Hide
        Andrew Stitcher added a comment -

        Soliciting opinion:

        This seems like it might actually be a blocking bug -

        1. It is a regression against 0.5 (lvq was introduced before 0.5 right?)
        2. It is impossible to work around - or is not using LVQ considered a workaround?

        Show
        Andrew Stitcher added a comment - Soliciting opinion: This seems like it might actually be a blocking bug - 1. It is a regression against 0.5 (lvq was introduced before 0.5 right?) 2. It is impossible to work around - or is not using LVQ considered a workaround?
        Hide
        Carl Trieloff added a comment -

        Here is a diff which I think resolves it, take a look and I will commit if not comments.

        Index: tests/QueueTest.cpp
        ===================================================================
        — tests/QueueTest.cpp (revision 895748)
        +++ tests/QueueTest.cpp (working copy)
        @@ -544,9 +544,14 @@
        framing::SequenceNumber sequence(1);
        QueuedMessage qmsg(queue.get(), msg1, sequence);
        QueuedMessage qmsg2(queue.get(), msg2, ++sequence);
        + framing::SequenceNumber sequence1(10);
        + QueuedMessage qmsg3(queue.get(), 0, sequence1);

        BOOST_CHECK(!queue->acquire(qmsg));
        BOOST_CHECK(queue->acquire(qmsg2));
        + // Acquire the massage again to test failure case.
        + BOOST_CHECK(!queue->acquire(qmsg2));
        + BOOST_CHECK(!queue->acquire(qmsg3));

        BOOST_CHECK_EQUAL(queue->getMessageCount(), 2u);

        Index: qpid/broker/Queue.cpp
        ===================================================================
        — qpid/broker/Queue.cpp (revision 895748)
        +++ qpid/broker/Queue.cpp (working copy)
        @@ -263,9 +263,10 @@
        Mutex::ScopedLock locker(messageLock);
        QPID_LOG(debug, "attempting to acquire " << msg.position);
        Messages::iterator i = findAt(msg.position);

        • if ((i != messages.end() && !lastValueQueue) // note that in some cases payload not be set
        • (lastValueQueue && (i->position == msg.position) &&
        • msg.payload.get() == checkLvqReplace(*i).payload.get()) )
          Unknown macro: {+ if ((i != messages.end() && msg.payload.get()) && // note that in some cases payload not be set+ (!lastValueQueue ||+ (lastValueQueue && (i->position == msg.position) && msg.payload.get() == checkLvqReplace(*i).payload.get()) )+ ) { clearLVQIndex(msg); QPID_LOG(debug, @@ -273,9 +274,7 @@ i->position << " == " << msg.position); messages.erase(i); return true; - } else { - QPID_LOG(debug, "No match: " << i->position << " != " << msg.position); - }+ }

        QPID_LOG(debug, "Acquire failed for " << msg.position);
        return false;

        Show
        Carl Trieloff added a comment - Here is a diff which I think resolves it, take a look and I will commit if not comments. Index: tests/QueueTest.cpp =================================================================== — tests/QueueTest.cpp (revision 895748) +++ tests/QueueTest.cpp (working copy) @@ -544,9 +544,14 @@ framing::SequenceNumber sequence(1); QueuedMessage qmsg(queue.get(), msg1, sequence); QueuedMessage qmsg2(queue.get(), msg2, ++sequence); + framing::SequenceNumber sequence1(10); + QueuedMessage qmsg3(queue.get(), 0, sequence1); BOOST_CHECK(!queue->acquire(qmsg)); BOOST_CHECK(queue->acquire(qmsg2)); + // Acquire the massage again to test failure case. + BOOST_CHECK(!queue->acquire(qmsg2)); + BOOST_CHECK(!queue->acquire(qmsg3)); BOOST_CHECK_EQUAL(queue->getMessageCount(), 2u); Index: qpid/broker/Queue.cpp =================================================================== — qpid/broker/Queue.cpp (revision 895748) +++ qpid/broker/Queue.cpp (working copy) @@ -263,9 +263,10 @@ Mutex::ScopedLock locker(messageLock); QPID_LOG(debug, "attempting to acquire " << msg.position); Messages::iterator i = findAt(msg.position); if ((i != messages.end() && !lastValueQueue) // note that in some cases payload not be set (lastValueQueue && (i->position == msg.position) && msg.payload.get() == checkLvqReplace(*i).payload.get()) ) Unknown macro: {+ if ((i != messages.end() && msg.payload.get()) && // note that in some cases payload not be set+ (!lastValueQueue ||+ (lastValueQueue && (i->position == msg.position) && msg.payload.get() == checkLvqReplace(*i).payload.get()) )+ ) { clearLVQIndex(msg); QPID_LOG(debug, @@ -273,9 +274,7 @@ i->position << " == " << msg.position); messages.erase(i); return true; - } else { - QPID_LOG(debug, "No match: " << i->position << " != " << msg.position); - }+ } QPID_LOG(debug, "Acquire failed for " << msg.position); return false;
        Hide
        Gordon Sim added a comment -

        As compared with the pre r834172 logic this removes a check for i->position == msg.position from the non-lvq case and adds in a test of msg.payload.get(). Is that deliberate?

        I think it would be worth adding a comment on what the extra lvq related logic is doing and why. Its not an easy piece of code to follow.

        Show
        Gordon Sim added a comment - As compared with the pre r834172 logic this removes a check for i->position == msg.position from the non-lvq case and adds in a test of msg.payload.get(). Is that deliberate? I think it would be worth adding a comment on what the extra lvq related logic is doing and why. Its not an easy piece of code to follow.
        Hide
        Carl Trieloff added a comment -

        Committed revision 896687.

        test and fix

        Show
        Carl Trieloff added a comment - Committed revision 896687. test and fix
        Carl Trieloff made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 0.7 [ 12314455 ]
        Resolution Fixed [ 1 ]
        Hide
        Gordon Sim added a comment -

        reopening to fix as blocker in 0.6

        Show
        Gordon Sim added a comment - reopening to fix as blocker in 0.6
        Gordon Sim made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Gordon Sim made changes -
        Priority Major [ 3 ] Blocker [ 1 ]
        Hide
        Gordon Sim added a comment -

        Commit r896687 merged onto release branch for 0.6 as r902969.

        Show
        Gordon Sim added a comment - Commit r896687 merged onto release branch for 0.6 as r902969.
        Gordon Sim made changes -
        Assignee Andrew Stitcher [ astitcher ]
        Hide
        Andrew Stitcher added a comment -

        Now been checked into 0.6 branch

        Show
        Andrew Stitcher added a comment - Now been checked into 0.6 branch
        Andrew Stitcher made changes -
        Status Reopened [ 4 ] Closed [ 6 ]
        Fix Version/s 0.6 [ 12313728 ]
        Resolution Fixed [ 1 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        2d 1h 55m 1 Carl Trieloff 06/Jan/10 21:50
        Resolved Resolved Reopened Reopened
        18d 22h 44m 1 Gordon Sim 25/Jan/10 20:35
        Reopened Reopened Closed Closed
        2d 18h 41m 1 Andrew Stitcher 28/Jan/10 15:16

          People

          • Assignee:
            Andrew Stitcher
            Reporter:
            Gordon Sim
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development