Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-81376

Race condition reading/writing QObjectPrivate::connectedSignals.

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: P3: Somewhat important
    • Resolution: Done
    • Affects Version/s: 5.12.2
    • Fix Version/s: 5.12.7
    • Component/s: Core: Event loop
    • Labels:
      None
    • Commits:
      567837a7422fe8d96011fbb3b23e7f34bc344e73 (qt/qtbase/5.12)

      Description

      QObjectPrivate::addConnection and QObjectPrivete::cleanConnectionLists may modify connectedSignals, and are correctly protected by a mutex. On the other hand, QObjectPrivate::isSignalConnected reads connectedSignals, but is NOT protected by a mutex.

      As these accesses are not atomic and there exists no happens-before relationship, the behavior is undefined and makes TSAN unhappy.

      One fix is to add a mutex, but they could also be made QAtomicIntegers (as in the patch below) Our benchmarking showed that there was a larger performance hit for the mutex option over the atomic.

      --- a/src/corelib/kernel/qobject.cpp
      +++ b/src/corelib/kernel/qobject.cpp
      @@ -232,7 +232,7 @@ QObjectPrivate::QObjectPrivate(int version)
           receiveChildEvents = true;
           postedEvents = 0;
           extraData = 0;
      -    connectedSignals[0] = connectedSignals[1] = 0;
      +    // connectedSignals[0] = connectedSignals[1] = 0;  // already 0.
           metaObject = 0;
           isWindow = false;
           deleteLaterCalled = false;
      @@ -411,9 +411,10 @@ void QObjectPrivate::addConnection(int signal, Connection *c)
               c->next->prev = &c->next;
       
           if (signal < 0) {
      -        connectedSignals[0] = connectedSignals[1] = ~0;
      +        connectedSignals[0].store(~0);
      +        connectedSignals[1].store(~0);
           } else if (signal < (int)sizeof(connectedSignals) * 8) {
      -        connectedSignals[signal >> 5] |= (1 << (signal & 0x1f));
      +        connectedSignals[signal >> 5].store(connectedSignals[signal >> 5].load() | (1 << (signal & 0x1f)));
           }
       }
       
      @@ -455,7 +456,7 @@ void QObjectPrivate::cleanConnectionLists()
                   if (!allConnected && !connected && signal >= 0
                       && size_t(signal) < sizeof(connectedSignals) * 8) {
                       // This signal is no longer connected
      -                connectedSignals[signal >> 5] &= ~(1 << (signal & 0x1f));
      +                connectedSignals[signal >> 5].store(connectedSignals[signal >> 5].load() & ~(1 << (signal & 0x1f)));
                   } else if (signal == -1) {
                       allConnected = connected;
                   }
      --- a/src/corelib/kernel/qobject_p.h
      +++ b/src/corelib/kernel/qobject_p.h
      @@ -232,7 +232,7 @@ public:
       
           Connection *senders;     // linked list of connections connected to this object
           Sender *currentSender;   // object currently activating the object
      -    mutable quint32 connectedSignals[2];
      +    mutable QAtomicInteger<quint32> connectedSignals[2];
       
           union {
               QObject *currentChildBeingDeleted; // should only be used when QObjectData::isDeletingChildren is set
      @@ -257,7 +257,7 @@ Q_DECLARE_TYPEINFO(QObjectPrivate::ConnectionList, Q_MOVABLE_TYPE);
       inline bool QObjectPrivate::isSignalConnected(uint signal_index, bool checkDeclarative) const
       {
           return signal_index >= sizeof(connectedSignals) * 8
      -        || (connectedSignals[signal_index >> 5] & (1 << (signal_index & 0x1f))
      +        || (connectedSignals[signal_index >> 5].load() & (1 << (signal_index & 0x1f))
               || (checkDeclarative && isDeclarativeSignalConnected(signal_index)));
       }
       
      
      

        Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

          Activity

            People

            Assignee:
            thiago Thiago Macieira
            Reporter:
            cwgthornton Chris Thornton
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Gerrit Reviews

                There are no open Gerrit changes