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

        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
        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.
        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
        Hide
        Carl Trieloff added a comment -

        Committed revision 896687.

        test and fix

        Show
        Carl Trieloff added a comment - Committed revision 896687. test and fix
        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 -

        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
        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?

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development