qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pavel Moravec" <pmora...@redhat.com>
Subject Review Request 31619: [C++ broker] segfault when processing QMF method during periodic processing
Date Mon, 02 Mar 2015 09:56:07 GMT

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

Review request for qpid, Andrew Stitcher, Gordon Sim, and Kenneth Giusti.


Bugs: QPID-6397
    https://issues.apache.org/jira/browse/QPID-6397


Repository: qpid


Description
-------

The race condition leading to the coredump with backtraces in the JIRA when:

- traces are enabled (at least for qpid::management::ManagementAgent::debugSnapshot scope)
- periodic processing has just started, is in debugSnapshot, and iterates either over managementObjects
(dumpMap) or over newManagementObjects (dumpVector)
- a QMF request is being processed via handleMethodRequest and moveNewObjects moves newly
seen objects from newManagementObjects to managementObjects

Here the problem is, access to neither managementObjects or newManagementObjects is guarded
by locks within debugSnapshot, causing the iteration over the map / vector fails when updating
it concurrently.

I spotted segfaults in either dumpMap or in dumpVector, both needs to be fixed.

Proposed is a trivial patch in adding both locks. An option (in case the locking is time consuming)
is conditional locks:

    Index: cpp/src/qpid/management/ManagementAgent.cpp
    ===================================================================
    --- cpp/src/qpid/management/ManagementAgent.cpp	(revision 1660046)
    +++ cpp/src/qpid/management/ManagementAgent.cpp	(working copy)
    @@ -2710,11 +2710,17 @@
                  << summarizeVector("new objects ", newManagementObjects)
                  << pendingDeletedObjs.size() << " pending deletes"
                  << summarizeAgents());
    -
    -    QPID_LOG_IF(trace, managementObjects.size(),
    -                title << ": objects" << dumpMap(managementObjects));
    -    QPID_LOG_IF(trace, newManagementObjects.size(),
    -                title << ": new objects" << dumpVector(newManagementObjects));
    +    bool print_traces;
    +    QPID_LOG_TEST(trace, print_traces);
    +    if (print_traces)
    +    {
    +        sys::Mutex::ScopedLock objLock (objectLock);
    +        sys::Mutex::ScopedLock lock(addLock);
    +        QPID_LOG_IF(trace, managementObjects.size(),
    +                    title << ": objects" << dumpMap(managementObjects));
    +        QPID_LOG_IF(trace, newManagementObjects.size(),
    +                    title << ": new objects" << dumpVector(newManagementObjects));
    +    }
     }

This might work well only if std::count_if on either map or vector (called in summarizeVector
and summarizeMap) is thread-safe operation.


Diffs
-----

  trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1660046 

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


Testing
-------

The first patch successfully tested against the reproducer for 3 days, the 2nd proposed hasnt
been tested.


Thanks,

Pavel Moravec


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message