Loading...

XML

Word

Printable

Details

Type: Task
Resolution: Fixed
Priority: P3: Somewhat important
Fix Version/s: 6.5.0 FF
Affects Version/s: None
Component/s: Network: SSL
Labels:
None

Sprint:
Foundation PM Prioritized, Foundation Sprint 62, Foundation Sprint 63, Foundation Sprint 64, Foundation Sprint 65, Foundation Sprint 66

Description

Recursive mutexes are much slower than ordinary ones.

The QTlsBackendOpenSSL::ensureCiphersAndCertsLoaded() function is called 1000s of times during e.g. tst_qsslcertificate_check, so it would be nice to use the Double-Checked Locking Pattern here to avoid having to take the mutex each time we call the function.

But a straight-forward port (items 2-3 below) isn't possible because the mutex is a recursive one. I tracked it down to the first QTlsBackendOpenSSL::ensureCiphersAndCertsLoaded() call recursing into itself (see frames #2 and #14 in the backtrace below), which, to this reporter, doesn't look trivial to avoid. But avoid we should.

Acceptance criteria:

code is fixed to not recurse into ensureCiphersAndCertsLoaded()
qt_opensslInitMutex is a static QBasicMutex instead of a Q_GLOBAL_STATIC(QRecursiveMutex)

QTlsBackendOpenSSL::ensureLibraryLoaded() and ensureCiphiersAndCertsLoaded() use double-checked locking to avoid locking the mutex once initialized, ie.

s_libraryLoaded is an atomic and checked before and after locking the mutex

if (s_libraryLoaded.load(std::memory_order::acquire))
    return;
const auto lock = qt_scoped_lock(qt_opensslInitMutex);
if (s_libraryLoaded.load(std::memory_order::acquire))
    return;
~~~~
s_libraryLoaded.store(true, std::memory_order::release);

1. ditto s_loadedCiphersAndCerts

Some random notes:

there's just the one mutex, but there doesn't seem to be overlap between the two functions using it, so maybe two mutexes would make sense here, in which case they could be function-statics in the functions that use them
the ensureLoaded() use looks like it could just use the thread-safe-statics-from-lambda trick:
```
static const bool inited = [&] {
    ~~~ initialization code ~~~
};
```
in which case the locking would be SEP (the compiler's)

Backtrace:

#2  QTlsBackendOpenSSL::ensureCiphersAndCertsLoaded (this=<optimized out>) at /home/marc/Qt/qt5/qtbase/src/plugins/tls/openssl/qtlsbackend_openssl.cpp:195
#3  0x00007ffff7ef076a in QSslSocketPrivate::ensureInitialized () at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslsocket.cpp:2019
#4  0x00007ffff7e68033 in QSslCertificatePrivate::QSslCertificatePrivate (this=0x488070) at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslcertificate.cpp:125
#5  QSslCertificate::QSslCertificate (this=0x451c08, data=..., format=QSsl::Pem) at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslcertificate.cpp:174
#6  0x00007ffff7ee6a3c in QSslConfigurationPrivate::QSslConfigurationPrivate (this=0x451c00) at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslconfiguration_p.h:49
#7  0x00007ffff7ef087d in QSslSocketGlobalData::QSslSocketGlobalData (this=0x7ffff7f5f600 <QGlobalStatic<QtGlobalStatic::Holder<(anonymous namespace)::Q_QGS_globalData> >::instance()::holder>)
    at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslsocket.cpp:372
#8  (anonymous namespace)::Q_QGS_globalData::innerFunction (pointer=0x7ffff7f5f600 <QGlobalStatic<QtGlobalStatic::Holder<(anonymous namespace)::Q_QGS_globalData> >::instance()::holder>)
    at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslsocket.cpp:386
#9  QtGlobalStatic::Holder<(anonymous namespace)::Q_QGS_globalData>::Holder (this=0x7ffff7f5f600 <QGlobalStatic<QtGlobalStatic::Holder<(anonymous namespace)::Q_QGS_globalData> >::instance()::holder>)
    at qtbase/include/QtCore/../../../../qt5/qtbase/src/corelib/global/qglobalstatic.h:37
#10 QGlobalStatic<QtGlobalStatic::Holder<(anonymous namespace)::Q_QGS_globalData> >::instance () at qtbase/include/QtCore/../../../../qt5/qtbase/src/corelib/global/qglobalstatic.h:91
#11 QGlobalStatic<QtGlobalStatic::Holder<(anonymous namespace)::Q_QGS_globalData> >::operator() (this=<optimized out>)
    at qtbase/include/QtCore/../../../../qt5/qtbase/src/corelib/global/qglobalstatic.h:73
#12 0x00007ffff7ef0ae3 in QSslSocketPrivate::setDefaultSupportedCiphers (ciphers=...) at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslsocket.cpp:2112
#13 0x00007ffff470d802 in QTlsBackendOpenSSL::resetDefaultCiphers () at /home/marc/Qt/qt5/qtbase/src/plugins/tls/openssl/qtlsbackend_openssl.cpp:253
#14 0x00007ffff470d37c in QTlsBackendOpenSSL::ensureCiphersAndCertsLoaded (this=<optimized out>) at /home/marc/Qt/qt5/qtbase/src/plugins/tls/openssl/qtlsbackend_openssl.cpp:201
#15 0x00007ffff7ef076a in QSslSocketPrivate::ensureInitialized () at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslsocket.cpp:2019
#16 0x00007ffff7e68033 in QSslCertificatePrivate::QSslCertificatePrivate (this=0x47e760) at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslcertificate.cpp:125
#17 QSslCertificate::QSslCertificate (this=0x7fffffffd1e0, data=..., format=QSsl::Pem) at /home/marc/Qt/qt5/qtbase/src/network/ssl/qsslcertificate.cpp:174
#18 0x0000000000407d50 in tst_QSslCertificate::hash (this=<optimized out>) at /home/marc/Qt/qt5/qtbase/tests/auto/network/ssl/qsslcertificate/tst_qsslcertificate.cpp:151
#19 0x00007ffff797bd25 in QMetaMethod::invoke (this=<optimized out>, object=<optimized out>, connectionType=<optimized out>, returnValue=..., val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., 
    val6=..., val7=..., val8=..., val9=...) at /home/marc/Qt/qt5/qtbase/src/corelib/kernel/qmetaobject.cpp:2357
#20 0x00007ffff7f83243 in QMetaMethod::invoke (this=0x7ffff473e937, object=0xffffffff, connectionType=Qt::DirectConnection, val0=..., val1=..., val2=..., val3=..., val4=..., val5=..., val6=..., 
    val7=..., val8=..., val9=...) at qtbase/include/QtCore/../../../../qt5/qtbase/src/corelib/kernel/qmetaobject.h:90
#21 QTest::TestMethods::invokeTestOnData (this=0x7fffffffda90, index=<optimized out>) at /home/marc/Qt/qt5/qtbase/src/testlib/qtestcase.cpp:1023
#22 0x00007ffff7f83f01 in QTest::TestMethods::invokeTest (this=0x7fffffffda90, index=0, tag=..., watchDog=<optimized out>) at /home/marc/Qt/qt5/qtbase/src/testlib/qtestcase.cpp:1282
#23 0x00007ffff7f84eee in QTest::TestMethods::invokeTests (this=<optimized out>, testObject=0x7fffffffdc58) at /home/marc/Qt/qt5/qtbase/src/testlib/qtestcase.cpp:1631
#24 0x00007ffff7f85885 in QTest::qRun () at /home/marc/Qt/qt5/qtbase/src/testlib/qtestcase.cpp:2100
#25 0x00007ffff7f851de in QTest::qExec (testObject=<optimized out>, argc=<optimized out>, argv=<optimized out>) at /home/marc/Qt/qt5/qtbase/src/testlib/qtestcase.cpp:2002
#26 0x0000000000420ae2 in main (argc=-1, argv=0x7fffffffddb8) at /home/marc/Qt/qt5/qtbase/tests/auto/network/ssl/qsslcertificate/tst_qsslcertificate.cpp:1468

Attachments

Gerrit Reviews

- Issue Only
- Show All Reviews
- Show Open Reviews
- Show All Issues
- Show Open Issues

For Gerrit Dashboard: QTBUG-103559
#	Subject	Branch	Project	Status	CR	V
429595,4	TLS backend OpenSSL: Use function-static variable for symbol loading	dev	qt/qtbase	Status: MERGED	+2	0
429671,4	QTlsBackendOpenSSL: Use a function-static variable in ensureLibraryLoaded()	dev	qt/qtbase	Status: MERGED	+2	0
430355,2	QTlsBackendOpenSSL: Make ensureLibraryLoaded() private	dev	qt/qtbase	Status: MERGED	+2	0