From 265db5ad9bda9c984393c1e95fd27dcc4633ed1c Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 14 Feb 2017 16:52:19 +0100 Subject: QJNIHelper: clean up atomic int and mutex handling 1. Do not use Q_GLOBAL_STATIC to hold QAtomicInt or QMutex, use file-static QBasicAtomicInt and QBasicMutex instead. They are zero-initialized PODs. 2. Use only QMutexLocker to lock mutexes. Also wrap the atomic counter into a next...() function, as done elsewhere. Change-Id: I4b14ac0de9d4cb6780b1f1372c2b5fc88e918e4c Reviewed-by: Olivier Goffart (Woboq GmbH) --- src/corelib/kernel/qjnihelpers.cpp | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) (limited to 'src/corelib/kernel/qjnihelpers.cpp') diff --git a/src/corelib/kernel/qjnihelpers.cpp b/src/corelib/kernel/qjnihelpers.cpp index 6a46f7dd11..2f701b8f0d 100644 --- a/src/corelib/kernel/qjnihelpers.cpp +++ b/src/corelib/kernel/qjnihelpers.cpp @@ -61,7 +61,7 @@ static jclass g_jNativeClass = Q_NULLPTR; static jmethodID g_runPendingCppRunnablesMethodID = Q_NULLPTR; static jmethodID g_hideSplashScreenMethodID = Q_NULLPTR; Q_GLOBAL_STATIC(std::deque, g_pendingRunnables); -Q_GLOBAL_STATIC(QMutex, g_pendingRunnablesMutex); +QBasicMutex g_pendingRunnablesMutex; class PermissionsResultClass : public QObject { @@ -76,21 +76,24 @@ private: typedef QHash> PendingPermissionRequestsHash; Q_GLOBAL_STATIC(PendingPermissionRequestsHash, g_pendingPermissionRequests); -Q_GLOBAL_STATIC(QMutex, g_pendingPermissionRequestsMutex); -Q_GLOBAL_STATIC(QAtomicInt, g_requestPermissionsRequestCode); +QBasicMutex g_pendingPermissionRequestsMutex; +static int nextRequestCode() +{ + static QBasicAtomicInt counter; + return counter.fetchAndAddRelaxed(0); +} // function called from Java from Android UI thread static void runPendingCppRunnables(JNIEnv */*env*/, jobject /*obj*/) { for (;;) { // run all posted runnables - g_pendingRunnablesMutex->lock(); + QMutexLocker locker(&g_pendingRunnablesMutex); if (g_pendingRunnables->empty()) { - g_pendingRunnablesMutex->unlock(); break; } QtAndroidPrivate::Runnable runnable(std::move(g_pendingRunnables->front())); g_pendingRunnables->pop_front(); - g_pendingRunnablesMutex->unlock(); + locker.unlock(); runnable(); // run it outside the sync block! } } @@ -110,14 +113,13 @@ Q_GLOBAL_STATIC(GenericMotionEventListeners, g_genericMotionEventListeners) static void sendRequestPermissionsResult(JNIEnv *env, jobject /*obj*/, jint requestCode, jobjectArray permissions, jintArray grantResults) { - g_pendingPermissionRequestsMutex->lock(); + QMutexLocker locker(&g_pendingPermissionRequestsMutex); auto it = g_pendingPermissionRequests->find(requestCode); if (it == g_pendingPermissionRequests->end()) { - g_pendingPermissionRequestsMutex->unlock(); // show an error or something ? return; } - g_pendingPermissionRequestsMutex->unlock(); + locker.unlock(); Qt::ConnectionType connection = QThread::currentThread() == it.value()->thread() ? Qt::DirectConnection : Qt::BlockingQueuedConnection; QtAndroidPrivate::PermissionsHash hash; @@ -132,9 +134,9 @@ static void sendRequestPermissionsResult(JNIEnv *env, jobject /*obj*/, jint requ hash[permission] = value; } QMetaObject::invokeMethod(it.value().data(), "sendResult", connection, Q_ARG(QtAndroidPrivate::PermissionsHash, hash)); - g_pendingPermissionRequestsMutex->lock(); + + locker.relock(); g_pendingPermissionRequests->erase(it); - g_pendingPermissionRequestsMutex->unlock(); } static jboolean dispatchGenericMotionEvent(JNIEnv *, jclass, jobject event) @@ -447,10 +449,10 @@ void QtAndroidPrivate::runOnUiThread(QRunnable *runnable, JNIEnv *env) void QtAndroidPrivate::runOnAndroidThread(const QtAndroidPrivate::Runnable &runnable, JNIEnv *env) { - g_pendingRunnablesMutex->lock(); + QMutexLocker locker(&g_pendingRunnablesMutex); const bool triggerRun = g_pendingRunnables->empty(); g_pendingRunnables->push_back(runnable); - g_pendingRunnablesMutex->unlock(); + locker.unlock(); if (triggerRun) env->CallStaticVoidMethod(g_jNativeClass, g_runPendingCppRunnablesMethodID); } @@ -475,18 +477,16 @@ void QtAndroidPrivate::requestPermissions(JNIEnv *env, const QStringList &permis return; } // Check API 23+ permissions - const int requestCode = (*g_requestPermissionsRequestCode)++; + const int requestCode = nextRequestCode(); if (!directCall) { - g_pendingPermissionRequestsMutex->lock(); + QMutexLocker locker(&g_pendingPermissionRequestsMutex); (*g_pendingPermissionRequests)[requestCode] = QSharedPointer::create(callbackFunc); - g_pendingPermissionRequestsMutex->unlock(); } runOnAndroidThread([permissions, callbackFunc, requestCode, directCall] { if (directCall) { - g_pendingPermissionRequestsMutex->lock(); + QMutexLocker locker(&g_pendingPermissionRequestsMutex); (*g_pendingPermissionRequests)[requestCode] = QSharedPointer::create(callbackFunc); - g_pendingPermissionRequestsMutex->unlock(); } QJNIEnvironmentPrivate env; -- cgit v1.2.3 From aebf66d242614334962a1d339ac6c168d9e3f9e6 Mon Sep 17 00:00:00 2001 From: Heikki Haveri Date: Thu, 12 Jan 2017 14:27:49 +0200 Subject: Enable QtAndroid::runOnAndroidThread in a Service Change-Id: I214f5dc70c52011a5e1712ea70f97f8b564fb664 Reviewed-by: BogDan Vatra --- src/corelib/kernel/qjnihelpers.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/corelib/kernel/qjnihelpers.cpp') diff --git a/src/corelib/kernel/qjnihelpers.cpp b/src/corelib/kernel/qjnihelpers.cpp index 2f701b8f0d..1ad6dfd44f 100644 --- a/src/corelib/kernel/qjnihelpers.cpp +++ b/src/corelib/kernel/qjnihelpers.cpp @@ -393,7 +393,7 @@ jint QtAndroidPrivate::initJNI(JavaVM *vm, JNIEnv *env) return JNI_ERR; g_runPendingCppRunnablesMethodID = env->GetStaticMethodID(jQtNative, - "runPendingCppRunnablesOnUiThread", + "runPendingCppRunnablesOnAndroidThread", "()V"); g_hideSplashScreenMethodID = env->GetStaticMethodID(jQtNative, "hideSplashScreen", "()V"); g_jNativeClass = static_cast(env->NewGlobalRef(jQtNative)); -- cgit v1.2.3 From 39820cf8c38f527e178913192d428085af4327b4 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 14 Feb 2017 17:04:55 +0100 Subject: QJNIHelper: fix a potential race in sendRequestPermissionsResult() The code obtained an iterator into a QHash under mutex protection, then dropped the lock, dereferenced the iterator several times and only retook the lock to erase the element from the QHash. This is very smelly. QHash provides no official iterator validity guarantees, and the container isn't const, either (which would imply thread-safety). In particular, the dereference into the container outside the critical section is cause for concerns. Simplify the code, removing any doubts about its race-freedom, by taking the payload item out of the hash before dropping the lock, and using only the local strong reference in the remainder of the function. The only other references to g_pendingPermissionRequests are insertions with unique-by-construction keys in QtAndroidPrivate's requestPermissions(), so there was no reason to keep the item in the hash for the whole duration of the sendRequestPermissionsResult() call. Change-Id: I39fe0803b13b3046d1f0fd9c8e96c531406d57da Reviewed-by: Olivier Goffart (Woboq GmbH) Reviewed-by: BogDan Vatra --- src/corelib/kernel/qjnihelpers.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'src/corelib/kernel/qjnihelpers.cpp') diff --git a/src/corelib/kernel/qjnihelpers.cpp b/src/corelib/kernel/qjnihelpers.cpp index 1ad6dfd44f..028fb1256e 100644 --- a/src/corelib/kernel/qjnihelpers.cpp +++ b/src/corelib/kernel/qjnihelpers.cpp @@ -119,9 +119,11 @@ static void sendRequestPermissionsResult(JNIEnv *env, jobject /*obj*/, jint requ // show an error or something ? return; } + auto request = std::move(*it); + g_pendingPermissionRequests->erase(it); locker.unlock(); - Qt::ConnectionType connection = QThread::currentThread() == it.value()->thread() ? Qt::DirectConnection : Qt::BlockingQueuedConnection; + Qt::ConnectionType connection = QThread::currentThread() == request->thread() ? Qt::DirectConnection : Qt::BlockingQueuedConnection; QtAndroidPrivate::PermissionsHash hash; const int size = env->GetArrayLength(permissions); std::unique_ptr results(new jint[size]); @@ -133,10 +135,7 @@ static void sendRequestPermissionsResult(JNIEnv *env, jobject /*obj*/, jint requ QtAndroidPrivate::PermissionsResult::Denied; hash[permission] = value; } - QMetaObject::invokeMethod(it.value().data(), "sendResult", connection, Q_ARG(QtAndroidPrivate::PermissionsHash, hash)); - - locker.relock(); - g_pendingPermissionRequests->erase(it); + QMetaObject::invokeMethod(request.data(), "sendResult", connection, Q_ARG(QtAndroidPrivate::PermissionsHash, hash)); } static jboolean dispatchGenericMotionEvent(JNIEnv *, jclass, jobject event) -- cgit v1.2.3 From af8771867c3bca9b1bc34d6d82b6603749938d22 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Tue, 14 Feb 2017 17:43:35 +0100 Subject: Fix UB (data race) in QtAndroidPrivate::requestPermissionsSync() If the QSemaphore::tryAcquire() call times out, we mustn't touch *res, because there was no happens-before relation established between *res = result in the lambda and our returning *res; Fix by returning a default-constructed hash in that case. Add a strategic std::move(). The same problem exists in runOnAndroidThreadSync(), but I have no idea how to solve it, because there the shared object is the runnable itself. Change-Id: I9a2c431144c169fbd545763555d96153143a11bf Reviewed-by: BogDan Vatra --- src/corelib/kernel/qjnihelpers.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src/corelib/kernel/qjnihelpers.cpp') diff --git a/src/corelib/kernel/qjnihelpers.cpp b/src/corelib/kernel/qjnihelpers.cpp index 028fb1256e..58912aa261 100644 --- a/src/corelib/kernel/qjnihelpers.cpp +++ b/src/corelib/kernel/qjnihelpers.cpp @@ -506,8 +506,10 @@ QHash QtAndroidPrivate::requestPer *res = result; sem->release(); }, true); - sem->tryAcquire(1, timeoutMs); - return *res; + if (sem->tryAcquire(1, timeoutMs)) + return std::move(*res); + else // mustn't touch *res + return QHash(); } QtAndroidPrivate::PermissionsResult QtAndroidPrivate::checkPermission(const QString &permission) -- cgit v1.2.3 From 03903ec7836958a1ddddff31960d5a1ef654b094 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Wed, 15 Feb 2017 13:50:46 +0100 Subject: QJNIHelpers: make mutexes static Amends 265db5ad9bda9c984393c1e95fd27dcc4633ed1c. Change-Id: I707bb88285531ee9f82efec46901871d53413eb3 Reviewed-by: BogDan Vatra --- src/corelib/kernel/qjnihelpers.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/corelib/kernel/qjnihelpers.cpp') diff --git a/src/corelib/kernel/qjnihelpers.cpp b/src/corelib/kernel/qjnihelpers.cpp index 58912aa261..310d3e24b2 100644 --- a/src/corelib/kernel/qjnihelpers.cpp +++ b/src/corelib/kernel/qjnihelpers.cpp @@ -61,7 +61,7 @@ static jclass g_jNativeClass = Q_NULLPTR; static jmethodID g_runPendingCppRunnablesMethodID = Q_NULLPTR; static jmethodID g_hideSplashScreenMethodID = Q_NULLPTR; Q_GLOBAL_STATIC(std::deque, g_pendingRunnables); -QBasicMutex g_pendingRunnablesMutex; +static QBasicMutex g_pendingRunnablesMutex; class PermissionsResultClass : public QObject { @@ -76,7 +76,7 @@ private: typedef QHash> PendingPermissionRequestsHash; Q_GLOBAL_STATIC(PendingPermissionRequestsHash, g_pendingPermissionRequests); -QBasicMutex g_pendingPermissionRequestsMutex; +static QBasicMutex g_pendingPermissionRequestsMutex; static int nextRequestCode() { static QBasicAtomicInt counter; -- cgit v1.2.3 From f05d2764b0c5a1b55c7856984017254b55bfc7e3 Mon Sep 17 00:00:00 2001 From: Marc Mutz Date: Thu, 16 Feb 2017 15:03:01 +0100 Subject: QJNIHelpers: unbreak runnables counter Adding 0 each time will obviously not produce a new identifier each time... Also use static initialization for QBasicAtomicInt. A default-constructed static QBasicAtomicInt at function scope will be dynamically initialized. It will still be zero-initialized, but at least GCC adds guard variables for such objects. When using aggregate initialization, the guard disappears. Amends 265db5ad9bda9c984393c1e95fd27dcc4633ed1c. Change-Id: Ia71290cf26c486dcbcc74381f12cd0c4712d6019 Reviewed-by: David Faure --- src/corelib/kernel/qjnihelpers.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/corelib/kernel/qjnihelpers.cpp') diff --git a/src/corelib/kernel/qjnihelpers.cpp b/src/corelib/kernel/qjnihelpers.cpp index 310d3e24b2..fe204bbee2 100644 --- a/src/corelib/kernel/qjnihelpers.cpp +++ b/src/corelib/kernel/qjnihelpers.cpp @@ -79,8 +79,8 @@ Q_GLOBAL_STATIC(PendingPermissionRequestsHash, g_pendingPermissionRequests); static QBasicMutex g_pendingPermissionRequestsMutex; static int nextRequestCode() { - static QBasicAtomicInt counter; - return counter.fetchAndAddRelaxed(0); + static QBasicAtomicInt counter = Q_BASIC_ATOMIC_INITIALIZER(0); + return counter.fetchAndAddRelaxed(1); } // function called from Java from Android UI thread -- cgit v1.2.3