From db0585d63ad44756c9b9476a53413ccd061c52d8 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Wed, 19 Jul 2017 23:32:30 -0700 Subject: Fix crashes with libproxy+webkitgtk: it's not threadsafe I'm getting crashes in Akonadi processes due to libproxy. I don't have direct evidence that this was caused by a threading condition, but it's clear from the source code of libproxy that the plugins it runs for expanding PAC scripts are not thread-safe. To overcome this problem, we only run libproxy functions in one thread only. #0 0x00007f745f0ac1d8 in JSC::HeapTimer::timerDidFire() () at /usr/lib64/libjavascriptcoregtk-4.0.so.18 #1 0x00007f745f0ac287 in () at /usr/lib64/libjavascriptcoregtk-4.0.so.18 #2 0x00007f748e5ae9c5 in g_main_context_dispatch () at /usr/lib64/libglib-2.0.so.0 #3 0x00007f748e5aed88 in () at /usr/lib64/libglib-2.0.so.0 #4 0x00007f748e5aee1c in g_main_context_iteration () at /usr/lib64/libglib-2.0.so.0 #5 0x00007f7494f4268f in QEventDispatcherGlib::processEvents(QFlags) () at /usr/lib64/libQt5Core.so.5 #6 0x00007f7494eeb35a in QEventLoop::exec(QFlags) () at /usr/lib64/libQt5Core.so.5 #7 0x00007f7494d1b31a in QThread::exec() () at /usr/lib64/libQt5Core.so.5 #8 0x00007f7494d1fd2e in () at /usr/lib64/libQt5Core.so.5 #9 0x00007f74913174e7 in start_thread () at /lib64/libpthread.so.0 The pacrunner implementation of libproxy uses libdbus-1 which (officially) is thread-safe, but experience tells that it has problems. Since it is not running a JS engine, we don't need a thread, but we do need to lock around it. Change-Id: I84e45059a888497fb55ffffd14d2f638f21e807d Reviewed-by: Edward Welbourne --- src/network/kernel/kernel.pri | 2 +- src/network/kernel/qnetworkproxy_libproxy.cpp | 143 +++++++++++++++++++++----- 2 files changed, 120 insertions(+), 25 deletions(-) (limited to 'src/network') diff --git a/src/network/kernel/kernel.pri b/src/network/kernel/kernel.pri index a80b2d387e..2c5975182f 100644 --- a/src/network/kernel/kernel.pri +++ b/src/network/kernel/kernel.pri @@ -63,6 +63,6 @@ osx:SOURCES += kernel/qnetworkproxy_mac.cpp else:win32:SOURCES += kernel/qnetworkproxy_win.cpp else: qtConfig(libproxy) { SOURCES += kernel/qnetworkproxy_libproxy.cpp - QMAKE_USE_PRIVATE += libproxy + QMAKE_USE_PRIVATE += libproxy libdl } else:SOURCES += kernel/qnetworkproxy_generic.cpp diff --git a/src/network/kernel/qnetworkproxy_libproxy.cpp b/src/network/kernel/qnetworkproxy_libproxy.cpp index 184dc6469d..29d2a0bd3b 100644 --- a/src/network/kernel/qnetworkproxy_libproxy.cpp +++ b/src/network/kernel/qnetworkproxy_libproxy.cpp @@ -1,6 +1,7 @@ /**************************************************************************** ** ** Copyright (C) 2016 The Qt Company Ltd. +** Copyright (C) 2017 Intel Corporation. ** Contact: https://www.qt.io/licensing/ ** ** This file is part of the QtNetwork module of the Qt Toolkit. @@ -42,57 +43,149 @@ #ifndef QT_NO_NETWORKPROXY #include +#include +#include #include +#include +#include #include +#include QT_BEGIN_NAMESPACE -class QLibProxyWrapper +static bool isThreadingNeeded() { -public: - QLibProxyWrapper() - : factory(px_proxy_factory_new()) - { - if (!factory) - qWarning("libproxy initialization failed."); - } + // Try to guess if the libproxy we linked to is from the libproxy project + // or if it is from pacrunner. Neither library is thread-safe, but the one + // from libproxy is worse, since it may launch JS engines that don't take + // kindly to being executed from multiple threads (even if at different + // times). The pacrunner implementation doesn't suffer from this because + // the JS execution is out of process, in the pacrunner daemon. - ~QLibProxyWrapper() - { - px_proxy_factory_free(factory); - } + void *sym; + +#ifdef Q_CC_GNU + // Search for the mangled name of the virtual table of the pacrunner + // extension. Even if libproxy begins using -fvisibility=hidden, this + // symbol can't be hidden. + sym = dlsym(RTLD_DEFAULT, "_ZTVN8libproxy19pacrunner_extensionE"); +#else + // The default libproxy one uses libmodman for its module management and + // leaks symbols because it doesn't use -fvisibility=hidden (as of + // v0.4.15). + sym = dlsym(RTLD_DEFAULT, "mm_info_ignore_hostname"); +#endif + + return sym != nullptr; +} + +class QLibProxyWrapper : public QDaemonThread +{ + Q_OBJECT +public: + QLibProxyWrapper(); + ~QLibProxyWrapper(); QList getProxies(const QUrl &url); private: - pxProxyFactory *factory; + struct Data { + // we leave the conversion to/from QUrl to the calling thread + const char *url; + char **proxies; + QSemaphore replyReady; + }; + + void run() override; + + pxProxyFactory *factory; // not subject to the mutex + + QMutex mutex; + QSemaphore requestReady; + Data *request; }; Q_GLOBAL_STATIC(QLibProxyWrapper, libProxyWrapper); +QLibProxyWrapper::QLibProxyWrapper() +{ + if (isThreadingNeeded()) { + setEventDispatcher(new QEventDispatcherUNIX); // don't allow the Glib one + start(); + } else { + factory = px_proxy_factory_new(); + Q_CHECK_PTR(factory); + } +} + +QLibProxyWrapper::~QLibProxyWrapper() +{ + if (isRunning()) { + requestInterruption(); + requestReady.release(); + wait(); + } else { + px_proxy_factory_free(factory); + } +} + /* - Gets the list of proxies from libproxy, converted to QUrl list. - Thread safe, according to libproxy documentation. + Gets the list of proxies from libproxy, converted to QUrl list. Apply + thread-safety, though its documentation says otherwise, libproxy isn't + thread-safe. */ QList QLibProxyWrapper::getProxies(const QUrl &url) { - QList ret; + QByteArray encodedUrl = url.toEncoded(); + Data data; + data.url = encodedUrl.constData(); + + { + QMutexLocker locker(&mutex); + if (isRunning()) { + // threaded mode + // it's safe to write to request because we hold the mutex: + // our aux thread is blocked waiting for work and no other thread + // could have got here + request = &data; + requestReady.release(); - if (factory) { - char **proxies = px_proxy_factory_get_proxies(factory, url.toEncoded()); - if (proxies) { - for (int i = 0; proxies[i]; i++) { - ret.append(QUrl::fromEncoded(proxies[i])); - free(proxies[i]); - } - free(proxies); + // wait for the reply + data.replyReady.acquire(); + } else { + // non-threaded mode + data.proxies = px_proxy_factory_get_proxies(factory, data.url); } } + QList ret; + if (data.proxies) { + for (int i = 0; data.proxies[i]; i++) { + ret.append(QUrl::fromEncoded(data.proxies[i])); + free(data.proxies[i]); + } + free(data.proxies); + } return ret; } +void QLibProxyWrapper::run() +{ + factory = px_proxy_factory_new(); + Q_CHECK_PTR(factory); + + forever { + requestReady.acquire(); + if (isInterruptionRequested()) + break; + request->proxies = px_proxy_factory_get_proxies(factory, request->url); + request->replyReady.release(); + } + + px_proxy_factory_free(factory); +} + QList QNetworkProxyFactory::systemProxyForQuery(const QNetworkProxyQuery &query) { QList proxyList; @@ -161,4 +254,6 @@ QList QNetworkProxyFactory::systemProxyForQuery(const QNetworkPro QT_END_NAMESPACE +#include "qnetworkproxy_libproxy.moc" + #endif -- cgit v1.2.3