From 570d00b000bd558592af317746fa3639fb022fa4 Mon Sep 17 00:00:00 2001
From: Nicolas Werner <nicolas.werner@hotmail.de>
Date: Sun, 25 Jul 2021 17:07:10 +0200
Subject: [PATCH] Fix crash when storing secrets

Nested QEventLoops are scary. Ultimately we shouldn't use them, but I
have no better solution right now.

fixes #656
---
 src/Cache.cpp | 45 +++++++++++++++++++++++----------------------
 src/Cache_p.h |  6 +++---
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/src/Cache.cpp b/src/Cache.cpp
index d651b182..4c24a712 100644
--- a/src/Cache.cpp
+++ b/src/Cache.cpp
@@ -715,32 +715,29 @@ Cache::restoreOlmAccount()
 }
 
 void
-Cache::storeSecret(const std::string &name, const std::string &secret)
+Cache::storeSecret(const std::string name, const std::string secret)
 {
         auto settings = UserSettings::instance();
-        QKeychain::WritePasswordJob job(QCoreApplication::applicationName());
-        job.setAutoDelete(false);
-        job.setInsecureFallback(true);
-        job.setKey("matrix." +
-                   QString(QCryptographicHash::hash(settings->profile().toUtf8(),
-                                                    QCryptographicHash::Sha256)) +
-                   "." + name.c_str());
-        job.setTextData(QString::fromStdString(secret));
-        QEventLoop loop;
-        job.connect(&job, &QKeychain::Job::finished, &loop, &QEventLoop::quit);
-        job.start();
-        loop.exec();
-
-        if (job.error()) {
-                nhlog::db()->warn(
-                  "Storing secret '{}' failed: {}", name, job.errorString().toStdString());
-        } else {
-                emit secretChanged(name);
-        }
+        auto job      = new QKeychain::WritePasswordJob(QCoreApplication::applicationName());
+        job->setInsecureFallback(true);
+        job->setKey("matrix." +
+                    QString(QCryptographicHash::hash(settings->profile().toUtf8(),
+                                                     QCryptographicHash::Sha256)) +
+                    "." + name.c_str());
+        job->setTextData(QString::fromStdString(secret));
+        QObject::connect(job, &QKeychain::Job::finished, job, [name, this](QKeychain::Job *job) {
+                if (job->error()) {
+                        nhlog::db()->warn(
+                          "Storing secret '{}' failed: {}", name, job->errorString().toStdString());
+                } else {
+                        emit secretChanged(name);
+                }
+        });
+        job->start();
 }
 
 void
-Cache::deleteSecret(const std::string &name)
+Cache::deleteSecret(const std::string name)
 {
         auto settings = UserSettings::instance();
         QKeychain::DeletePasswordJob job(QCoreApplication::applicationName());
@@ -750,6 +747,8 @@ Cache::deleteSecret(const std::string &name)
                    QString(QCryptographicHash::hash(settings->profile().toUtf8(),
                                                     QCryptographicHash::Sha256)) +
                    "." + name.c_str());
+        // FIXME(Nico): Nested event loops are dangerous. Some other slots may resume in the mean
+        // time!
         QEventLoop loop;
         job.connect(&job, &QKeychain::Job::finished, &loop, &QEventLoop::quit);
         job.start();
@@ -759,7 +758,7 @@ Cache::deleteSecret(const std::string &name)
 }
 
 std::optional<std::string>
-Cache::secret(const std::string &name)
+Cache::secret(const std::string name)
 {
         auto settings = UserSettings::instance();
         QKeychain::ReadPasswordJob job(QCoreApplication::applicationName());
@@ -769,6 +768,8 @@ Cache::secret(const std::string &name)
                    QString(QCryptographicHash::hash(settings->profile().toUtf8(),
                                                     QCryptographicHash::Sha256)) +
                    "." + name.c_str());
+        // FIXME(Nico): Nested event loops are dangerous. Some other slots may resume in the mean
+        // time!
         QEventLoop loop;
         job.connect(&job, &QKeychain::Job::finished, &loop, &QEventLoop::quit);
         job.start();
diff --git a/src/Cache_p.h b/src/Cache_p.h
index c9d42202..89c88925 100644
--- a/src/Cache_p.h
+++ b/src/Cache_p.h
@@ -285,9 +285,9 @@ public:
         void saveOlmAccount(const std::string &pickled);
         std::string restoreOlmAccount();
 
-        void storeSecret(const std::string &name, const std::string &secret);
-        void deleteSecret(const std::string &name);
-        std::optional<std::string> secret(const std::string &name);
+        void storeSecret(const std::string name, const std::string secret);
+        void deleteSecret(const std::string name);
+        std::optional<std::string> secret(const std::string name);
 
         template<class T>
         static constexpr bool isStateEvent(const mtx::events::StateEvent<T> &)