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

Using two different QSslSockets in two different threads will crash because of const-corectness issues in QSslCertificate

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: P2: Important
    • Resolution: Done
    • Affects Version/s: 4.8.0
    • Fix Version/s: 4.8.1, 5.0.0
    • Component/s: Network: SSL
    • Labels:
      None
    • Commits:
      656fab5e848fd14e5d00536a4babbb2f33dbcfb7 (4.8) 00821ec710ec8d9549b9f7e68837a7393a954754 (5.0)

      Description

      1. Suppose the app developer has two threads with two different instances of QSSlSocket

      2. Each thread calls QSslSocket::startClientEncryption(). So far so good.

      3. Inside startClientEncryption(), further down the call chain, at line 332 in qsslsocket_openssl.cpp, we iterate over the certificates returned from QSSlSocket::caCertificates(), and we check if they are valid. Those certificates are shared between instances, but since QSslCertificate::isValid() is supposed to be a const operation, there should be no harm in doing this.

      The problem is: QSslCertificate::isValid() actually mutates itself

      Why?
      Because after the Comodo incident back in March, QSslCertificate::isValid() now calls isBlacklisted() wich calls serialNumber() which calculates, stores, and returns the serial number. This is a non-const operation on the d-pointer, but since it happens in a const method, the d-pointer is not being detached, ultimately causing a crash since the other thread is doing the same thing.

      This is a problem with many methods of QSslCertificate that are marked as const but actually aren't since they use this calculate-and-store-result-on-first-use idiom. If they where using the Q_D macro, this would have been caught by the compiler as a const-corectness error.

        Attachments

          Issue Links

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

            Activity

              People

              Assignee:
              shkearns Shane Kearns
              Reporter:
              gregschlom Gregory Schlomoff
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Gerrit Reviews

                  There are no open Gerrit changes