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) <ogoffart@woboq.com>
Reviewed-by: BogDan Vatra <bogdan@kdab.com>

1 files changed, 4 insertions, 5 deletions

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<jint[]> 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)