Qpid
  1. Qpid
  2. QPID-3877

Modifying a message's headers may cause a broker crash.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.16
    • Fix Version/s: 0.16
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      Adjusting the message's TTL header can cause a crash if another thread is attempting to encode the same message. The below trace is from valgrind while the store is in use:

      ==15071== Thread 4:
      ==15071== Invalid read of size 8
      ==15071== at 0x52661F0: qpid::framing::DeliveryProperties::bodySize() const (DeliveryProperties.cpp:198)
      ==15071== by 0x5266248: qpid::framing::DeliveryProperties::encodedSize() const (DeliveryProperties.cpp:209)
      ==15071== by 0x52CC3D4: qpid::framing::AMQHeaderBody::encodedSize() const (AMQHeaderBody.h:45)
      ==15071== by 0x52CAFEE: qpid::framing::AMQFrame::encodedSize() const (AMQFrame.cpp:46)
      ==15071== by 0x4DB3B55: qpid::broker::Message::encodedHeaderSize() const (frame_functors.h:39)
      ==15071== by 0x5C1FBFC: mrg::msgstore::MessageStoreImpl::msgEncode(std::vector<char, std::allocator<char> >&, boost::intrusive_ptr<qpid::broker::PersistableMessage> const&) (Messag\
      eStoreImpl.cpp:1311)
      ==15071== by 0x5C31E8D: mrg::msgstore::MessageStoreImpl::store(qpid::broker::PersistableQueue const*, mrg::msgstore::TxnCtxt*, boost::intrusive_ptr<qpid::broker::PersistableMessage\
      > const&, bool) (MessageStoreImpl.cpp:1331)
      ==15071== by 0x5C32A0B: mrg::msgstore::MessageStoreImpl::enqueue(qpid::broker::TransactionContext*, boost::intrusive_ptr<qpid::broker::PersistableMessage> const&, qpid::broker::Per\
      sistableQueue const&) (MessageStoreImpl.cpp:1303)
      ==15071== by 0x4DBE65F: qpid::broker::MessageStoreModule::enqueue(qpid::broker::TransactionContext*, boost::intrusive_ptr<qpid::broker::PersistableMessage> const&, qpid::broker::Pe\
      rsistableQueue const&) (MessageStoreModule.cpp:125)
      ==15071== by 0x4DCFF31: qpid::broker::Queue::enqueue(qpid::broker::TransactionContext*, boost::intrusive_ptr<qpid::broker::Message>&, bool) (Queue.cpp:811)
      ==15071== by 0x4DD1951: qpid::broker::Queue::deliver(boost::intrusive_ptr<qpid::broker::Message>) (Queue.cpp:171)
      ==15071== by 0x4D798DE: qpid::broker::DeliverableMessage::deliverTo(boost::shared_ptr<qpid::broker::Queue> const&) (DeliverableMessage.cpp:33)
      ==15071== Address 0xca1d548 is 56 bytes inside a block of size 248 free'd
      ==15071== at 0x4A0545F: operator delete(void*) (vg_replace_malloc.c:387)
      ==15071== by 0x52CB195: qpid::framing::AMQFrame::cloneBody() (RefCounted.h:42)
      ==15071== by 0x4DB37DE: qpid::broker::Message::getHeaderBody() (Message.cpp:351)
      ==15071== by 0x4DB7471: qpid::framing::DeliveryProperties* qpid::broker::Message::getModifiableProperties<qpid::framing::DeliveryProperties>() (Message.h:208)
      ==15071== by 0x4DB65E7: qpid::broker::Message::adjustTtl() (Message.cpp:416)
      ==15071== by 0x4D7BB60: qpid::broker::DeliveryRecord::deliver(qpid::framing::Handler<qpid::framing::AMQFrame&>&, qpid::framing::SequenceNumber, unsigned short) (DeliveryRecord.cpp:\
      80)
      ==15071== by 0x4E1A9B9: qpid::broker::SessionState::deliver(qpid::broker::DeliveryRecord&, bool) (SessionState.cpp:380)
      ==15071== by 0x4DFD345: qpid::broker::SemanticState::ConsumerImpl::deliver(qpid::broker::QueuedMessage&) (SemanticState.cpp:342)
      ==15071== by 0x4DD519C: qpid::broker::Queue::dispatch(boost::shared_ptr<qpid::broker::Consumer>) (Queue.cpp:393)
      ==15071== by 0x4E00759: qpid::broker::SemanticState::ConsumerImpl::doOutput() (SemanticState.cpp:741)
      ==15071== by 0x52F174C: qpid::sys::AggregateOutput::doOutput() (AggregateOutput.cpp:59)
      ==15071== by 0x4D6FC18: qpid::broker::Connection::doOutput() (Connection.cpp:354)
      ==15071==

      Gordon correctly points out that the message lock must be held while the headers are being encoded.

        Activity

        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4150/
        -----------------------------------------------------------

        Review request for qpid, Gordon Sim and Kim van der Riet.

        Summary
        -------

        While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same.

        To reproduce:

        1) run broker with async store:
        qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW

        2) run qpid-perftest in a loop against the broker:
        while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done

        This addresses bug qpid-3877.
        https://issues.apache.org/jira/browse/qpid-3877

        Diffs


        /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1295759

        Diff: https://reviews.apache.org/r/4150/diff

        Testing
        -------

        Ran the above tests without crash.
        +make check.

        Thanks,

        Kenneth

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4150/ ----------------------------------------------------------- Review request for qpid, Gordon Sim and Kim van der Riet. Summary ------- While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same. To reproduce: 1) run broker with async store: qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW 2) run qpid-perftest in a loop against the broker: while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done This addresses bug qpid-3877. https://issues.apache.org/jira/browse/qpid-3877 Diffs /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1295759 Diff: https://reviews.apache.org/r/4150/diff Testing ------- Ran the above tests without crash. +make check. Thanks, Kenneth
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4150/#review5550
        -----------------------------------------------------------

        Ship it!

        • Gordon

        On 2012-03-02 02:14:07, Kenneth Giusti wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4150/

        -----------------------------------------------------------

        (Updated 2012-03-02 02:14:07)

        Review request for qpid, Gordon Sim and Kim van der Riet.

        Summary

        -------

        While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same.

        To reproduce:

        1) run broker with async store:

        qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW

        2) run qpid-perftest in a loop against the broker:

        while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done

        This addresses bug qpid-3877.

        https://issues.apache.org/jira/browse/qpid-3877

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1295759

        Diff: https://reviews.apache.org/r/4150/diff

        Testing

        -------

        Ran the above tests without crash.

        +make check.

        Thanks,

        Kenneth

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4150/#review5550 ----------------------------------------------------------- Ship it! Gordon On 2012-03-02 02:14:07, Kenneth Giusti wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4150/ ----------------------------------------------------------- (Updated 2012-03-02 02:14:07) Review request for qpid, Gordon Sim and Kim van der Riet. Summary ------- While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same. To reproduce: 1) run broker with async store: qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW 2) run qpid-perftest in a loop against the broker: while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done This addresses bug qpid-3877. https://issues.apache.org/jira/browse/qpid-3877 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1295759 Diff: https://reviews.apache.org/r/4150/diff Testing ------- Ran the above tests without crash. +make check. Thanks, Kenneth
        Hide
        Ken Giusti added a comment -

        Fix appears incomplete: crash will occasionally occur when encoding the message body (as opposed to the headers):

        qpidd crashes while qpid-perftest --mode fanout or topic in
        qpid::broker::Message::encode() -> map_if<> -> operator():

        (gdb) 6 Thread 0x2b6379e02040 (LWP 16744) 0x00000036088d4d98 in epoll_wait
        ()
        from /lib64/libc.so.6
        5 Thread 16745 0x000000360900b1c0 in pthread_cond_timedwait@@GLIBC_2.3.2
        ()
        from /lib64/libpthread.so.0
        4 Thread 16746 0x00000036088d4d98 in epoll_wait () from /lib64/libc.so.6
        3 Thread 16748 0x00000036088d4d98 in epoll_wait () from /lib64/libc.so.6
        2 Thread 16749 0x00000036088c6c2b in write () from /lib64/libc.so.6

        • 1 Thread 0x42e13940 (LWP 16747) 0x00002aaab40158b0 in ?? ()
          ...
          Thread 1 (Thread 0x42e13940 (LWP 16747)):
          #0 0x00002aaab40158b0 in ?? ()
          #1 0x000000360bbaa358 in operator() (this=<value optimized out>, buffer=...)
          at qpid/framing/TypeFilter.h:38
          #2 map_if<qpid::framing::EncodeBody, qpid::framing::TypeFilter<3u> > (
          this=<value optimized out>, buffer=...) at qpid/framing/FrameSet.h:110
          #3 qpid::broker::Message::encode (this=<value optimized out>, buffer=...)
          at qpid/broker/Message.cpp:143
          #4 0x00002b637c262583 in mrg::msgstore::MessageStoreImpl::msgEncode (
          this=<value optimized out>,
          buff=std::vector of length 160, capacity 160 = {...}

          , message=...)
          at MessageStoreImpl.cpp:1321
          #5 0x00002b637c262bf7 in mrg::msgstore::MessageStoreImpl::store (

        Show
        Ken Giusti added a comment - Fix appears incomplete: crash will occasionally occur when encoding the message body (as opposed to the headers): qpidd crashes while qpid-perftest --mode fanout or topic in qpid::broker::Message::encode() -> map_if<> -> operator(): (gdb) 6 Thread 0x2b6379e02040 (LWP 16744) 0x00000036088d4d98 in epoll_wait () from /lib64/libc.so.6 5 Thread 16745 0x000000360900b1c0 in pthread_cond_timedwait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0 4 Thread 16746 0x00000036088d4d98 in epoll_wait () from /lib64/libc.so.6 3 Thread 16748 0x00000036088d4d98 in epoll_wait () from /lib64/libc.so.6 2 Thread 16749 0x00000036088c6c2b in write () from /lib64/libc.so.6 1 Thread 0x42e13940 (LWP 16747) 0x00002aaab40158b0 in ?? () ... Thread 1 (Thread 0x42e13940 (LWP 16747)): #0 0x00002aaab40158b0 in ?? () #1 0x000000360bbaa358 in operator() (this=<value optimized out>, buffer=...) at qpid/framing/TypeFilter.h:38 #2 map_if<qpid::framing::EncodeBody, qpid::framing::TypeFilter<3u> > ( this=<value optimized out>, buffer=...) at qpid/framing/FrameSet.h:110 #3 qpid::broker::Message::encode (this=<value optimized out>, buffer=...) at qpid/broker/Message.cpp:143 #4 0x00002b637c262583 in mrg::msgstore::MessageStoreImpl::msgEncode ( this=<value optimized out>, buff=std::vector of length 160, capacity 160 = {...} , message=...) at MessageStoreImpl.cpp:1321 #5 0x00002b637c262bf7 in mrg::msgstore::MessageStoreImpl::store (
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4150/
        -----------------------------------------------------------

        (Updated 2012-03-20 13:23:19.671544)

        Review request for qpid, Gordon Sim and Kim van der Riet.

        Changes
        -------

        Crash also occurs during message body encode. Proposed fix simply moves scope of lock to include the message body.

        Summary
        -------

        While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same.

        To reproduce:

        1) run broker with async store:
        qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW

        2) run qpid-perftest in a loop against the broker:
        while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done

        This addresses bug qpid-3877.
        https://issues.apache.org/jira/browse/qpid-3877

        Diffs (updated)


        /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1302629

        Diff: https://reviews.apache.org/r/4150/diff

        Testing
        -------

        Ran the above tests without crash.
        +make check.

        Thanks,

        Kenneth

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4150/ ----------------------------------------------------------- (Updated 2012-03-20 13:23:19.671544) Review request for qpid, Gordon Sim and Kim van der Riet. Changes ------- Crash also occurs during message body encode. Proposed fix simply moves scope of lock to include the message body. Summary ------- While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same. To reproduce: 1) run broker with async store: qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW 2) run qpid-perftest in a loop against the broker: while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done This addresses bug qpid-3877. https://issues.apache.org/jira/browse/qpid-3877 Diffs (updated) /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1302629 Diff: https://reviews.apache.org/r/4150/diff Testing ------- Ran the above tests without crash. +make check. Thanks, Kenneth
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4150/
        -----------------------------------------------------------

        (Updated 2012-03-20 15:23:56.599695)

        Review request for qpid, Andrew Stitcher, Gordon Sim, and Kim van der Riet.

        Changes
        -------

        Added Andrew.

        Summary
        -------

        While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same.

        To reproduce:

        1) run broker with async store:
        qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW

        2) run qpid-perftest in a loop against the broker:
        while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done

        This addresses bug qpid-3877.
        https://issues.apache.org/jira/browse/qpid-3877

        Diffs


        /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1302629

        Diff: https://reviews.apache.org/r/4150/diff

        Testing
        -------

        Ran the above tests without crash.
        +make check.

        Thanks,

        Kenneth

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4150/ ----------------------------------------------------------- (Updated 2012-03-20 15:23:56.599695) Review request for qpid, Andrew Stitcher, Gordon Sim, and Kim van der Riet. Changes ------- Added Andrew. Summary ------- While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same. To reproduce: 1) run broker with async store: qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW 2) run qpid-perftest in a loop against the broker: while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done This addresses bug qpid-3877. https://issues.apache.org/jira/browse/qpid-3877 Diffs /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1302629 Diff: https://reviews.apache.org/r/4150/diff Testing ------- Ran the above tests without crash. +make check. Thanks, Kenneth
        Hide
        jiraposter@reviews.apache.org added a comment -

        -----------------------------------------------------------
        This is an automatically generated e-mail. To reply, visit:
        https://reviews.apache.org/r/4150/#review6120
        -----------------------------------------------------------

        Ship it!

        • Gordon

        On 2012-03-20 15:23:56, Kenneth Giusti wrote:

        -----------------------------------------------------------

        This is an automatically generated e-mail. To reply, visit:

        https://reviews.apache.org/r/4150/

        -----------------------------------------------------------

        (Updated 2012-03-20 15:23:56)

        Review request for qpid, Andrew Stitcher, Gordon Sim, and Kim van der Riet.

        Summary

        -------

        While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same.

        To reproduce:

        1) run broker with async store:

        qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW

        2) run qpid-perftest in a loop against the broker:

        while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done

        This addresses bug qpid-3877.

        https://issues.apache.org/jira/browse/qpid-3877

        Diffs

        -----

        /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1302629

        Diff: https://reviews.apache.org/r/4150/diff

        Testing

        -------

        Ran the above tests without crash.

        +make check.

        Thanks,

        Kenneth

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4150/#review6120 ----------------------------------------------------------- Ship it! Gordon On 2012-03-20 15:23:56, Kenneth Giusti wrote: ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4150/ ----------------------------------------------------------- (Updated 2012-03-20 15:23:56) Review request for qpid, Andrew Stitcher, Gordon Sim, and Kim van der Riet. Summary ------- While running the broker with an async store, it is possible to crash the broker using perftest. This is due to a race condition where the broker is updating the message's headers (ttl) while the store is encoding the same. To reproduce: 1) run broker with async store: qpidd --load-module /home/kgiusti/store/cpp/lib/.libs/msgstore.so --auth no --port 8888 --num-jfile 16 --jfile-size-pgs 128 --data-dir /tmp/tmp.364FDjf0YW 2) run qpid-perftest in a loop against the broker: while true; do qpid-perftest --port 8888 --mode fanout --count 25000 --size 256 --durable yes --nsubs 4 ; done This addresses bug qpid-3877. https://issues.apache.org/jira/browse/qpid-3877 Diffs ----- /trunk/qpid/cpp/src/qpid/broker/Message.cpp 1302629 Diff: https://reviews.apache.org/r/4150/diff Testing ------- Ran the above tests without crash. +make check. Thanks, Kenneth
        Hide
        Justin Ross added a comment -

        Reviewed by Gordon. Approved for 0.16.

        Show
        Justin Ross added a comment - Reviewed by Gordon. Approved for 0.16.

          People

          • Assignee:
            Ken Giusti
            Reporter:
            Ken Giusti
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development