fix: make ipc client thread-safe (#2075)

client/core/ipcclient.cpp

@@ -3,6 +3,11 @@
 #include <QRemoteObjectNode>
 #include <QtNetwork/qlocalsocket.h>
 
+namespace
+{
+    thread_local IpcClient ipcClient;
+}
+
 IpcClient::IpcClient(QObject *parent) : QObject(parent)
 {
     m_localSocket.setServerName(amnezia::getIpcServiceUrl());
@@ -14,7 +19,7 @@ IpcClient::IpcClient(QObject *parent) : QObject(parent)
         m_isSocketConnected = true;
     });
 
-    connect(&m_localSocket, &QLocalSocket::disconnected,  [this]() {
+    connect(&m_localSocket, &QLocalSocket::disconnected, this, [this]() {
         m_ipcClient.clear();
         m_Tun2SocksClient.clear();
         m_isSocketConnected = false;
@@ -23,22 +28,18 @@ IpcClient::IpcClient(QObject *parent) : QObject(parent)
 
 IpcClient *IpcClient::Instance()
 {
-    static IpcClient instance;
+    if (!ipcClient.m_isSocketConnected) {
-
+        ipcClient.establishConnection();
-    QMutexLocker locker(&instance.m_mutex);
-
-    if (!instance.m_isSocketConnected) {
-        instance.establishConnection();
     }
 
-    return &instance;
+    return &ipcClient;
 }
 
 QSharedPointer<IpcInterfaceReplica> IpcClient::Interface()
 {
-    auto rep = Instance()->m_ipcClient;
+    QSharedPointer<IpcInterfaceReplica> rep = Instance()->m_ipcClient;
-    if (!rep) {
+    if (rep.isNull()) {
-        qCritical() << "IpcClient::Interface(): Replica is undefined";
+        qCritical() << "IpcClient::Interface(): Failed to acquire replica";
         return nullptr;
     }
     if (!rep->waitForSource(1000)) {
@@ -53,8 +54,8 @@ QSharedPointer<IpcInterfaceReplica> IpcClient::Interface()
 
 QSharedPointer<IpcProcessTun2SocksReplica> IpcClient::InterfaceTun2Socks()
 {
-    auto rep = Instance()->m_Tun2SocksClient;
+    QSharedPointer<IpcProcessTun2SocksReplica> rep = Instance()->m_Tun2SocksClient;
-    if (!rep) {
+    if (rep.isNull()) {
         qCritical() << "IpcClient::InterfaceTun2Socks: Replica is undefined";
         return nullptr;
     }
@@ -76,19 +77,20 @@ bool IpcClient::establishConnection()
 
 QSharedPointer<PrivilegedProcess> IpcClient::CreatePrivilegedProcess()
 {
-    auto rep = Interface();
+    QSharedPointer<IpcInterfaceReplica> rep = Interface();
     if (!rep) {
-        qCritical() << "IpcClient::createPrivilegedProcess : IpcClient IpcClient replica is not valid";
+        qCritical() << "IpcClient::createPrivilegedProcess: Replica is invalid";
         return nullptr;
     }
 
-    QRemoteObjectPendingReply<int> futureResult = rep->createPrivilegedProcess();
+    QRemoteObjectPendingReply<int> pidReply = rep->createPrivilegedProcess();
-    futureResult.waitForFinished(5000);
+    if (!pidReply.waitForFinished(5000)){
+        qCritical() << "IpcClient::createPrivilegedProcess: Failed to execute RO createPrivilegedProcess call";
+        return nullptr;
+    }
 
-    int pid = futureResult.returnValue();
+    int pid = pidReply.returnValue();
-
+    QSharedPointer<ProcessDescriptor> pd(new ProcessDescriptor());
-    auto pd = QSharedPointer<ProcessDescriptor>(new ProcessDescriptor());
-    Instance()->m_processNodes.insert(pid, pd);
 
     pd->localSocket.reset(new QLocalSocket(pd->replicaNode.data()));
 
@@ -96,6 +98,7 @@ QSharedPointer<PrivilegedProcess> IpcClient::CreatePrivilegedProcess()
         pd->replicaNode->addClientSideConnection(pd->localSocket.data());
 
         IpcProcessInterfaceReplica *repl = pd->replicaNode->acquire<IpcProcessInterfaceReplica>();
+        // TODO: rework the unsafe cast below
         PrivilegedProcess *priv = static_cast<PrivilegedProcess *>(repl);
         pd->ipcProcess.reset(priv);
         if (!pd->ipcProcess) {
@@ -110,8 +113,12 @@ QSharedPointer<PrivilegedProcess> IpcClient::CreatePrivilegedProcess()
                              [pd]() { pd->replicaNode->deleteLater(); });
         }
     });
+
     pd->localSocket->connectToServer(amnezia::getIpcProcessUrl(pid));
-    pd->localSocket->waitForConnected();
+    if (!pd->localSocket->waitForConnected()) {
+        qCritical() << "IpcClient::createPrivilegedProcess: Failed to connect to process' socket";
+        return nullptr;
+    }
 
     auto processReplica = QSharedPointer<PrivilegedProcess>(pd->ipcProcess);
     return processReplica;

client/core/ipcclient.h

@@ -27,7 +27,6 @@ signals:
 private:
     bool establishConnection();
 
-    QMutex m_mutex;
     QLocalSocket m_localSocket;
     QRemoteObjectNode m_ClientNode;
     QSharedPointer<IpcInterfaceReplica> m_ipcClient;
@@ -44,7 +43,6 @@ private:
         QSharedPointer<QLocalSocket> localSocket;
     };
 
-    QMap<int, QSharedPointer<ProcessDescriptor>> m_processNodes;
     bool m_isSocketConnected {false};
 };
 

ipc/ipcserver.cpp

@@ -147,35 +147,64 @@ void IpcServer::cleanUp()
 
 void IpcServer::clearLogs()
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::clearLogs";
+#endif
+
     Logger::clearLogs(true);
 }
 
 bool IpcServer::createTun(const QString &dev, const QString &subnet)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::createTun";
+#endif
+
     return Router::createTun(dev, subnet);
 }
 
 bool IpcServer::deleteTun(const QString &dev)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::deleteTun";
+#endif
+
     return Router::deleteTun(dev);
 }
 
 bool IpcServer::updateResolvers(const QString &ifname, const QList<QHostAddress> &resolvers)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::updateResolvers";
+#endif
+
     return Router::updateResolvers(ifname, resolvers);
 }
 
-bool IpcServer::restoreResolvers() {
+bool IpcServer::restoreResolvers()
+{
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::restoreResolvers";
+#endif
+
     return Router::restoreResolvers();
 }
 
 bool IpcServer::StartRoutingIpv6()
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::StartRoutingIpv6";
+#endif
+
     return Router::StartRoutingIpv6();
 }
 
 bool IpcServer::StopRoutingIpv6()
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::StopRoutingIpv6";
+#endif
+
     return Router::StopRoutingIpv6();
 }
 
@@ -194,59 +223,101 @@ void IpcServer::setLogsEnabled(bool enabled)
 
 bool IpcServer::startNetworkCheck(const QString& serverIpv4Gateway, const QString& deviceIpv4Address)
 {
-    qDebug() << "startNetworkCheck";
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::startNetworkCheck";
+#endif
+
     m_pingHelper.start(serverIpv4Gateway, deviceIpv4Address);
     return true;
 }
 
 bool IpcServer::stopNetworkCheck()
 {
-    qDebug() << "stopNetworkCheck";
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::stopNetworkCheck";
+#endif
+
     m_pingHelper.stop();
     return true;
 }
 
 bool IpcServer::resetKillSwitchAllowedRange(QStringList ranges)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::resetKillSwitchAllowedRange";
+#endif
+
     return KillSwitch::instance()->resetAllowedRange(ranges);
 }
 
 bool IpcServer::addKillSwitchAllowedRange(QStringList ranges)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::addKillSwitchAllowedRange";
+#endif
+
     return KillSwitch::instance()->addAllowedRange(ranges);
 }
 
 bool IpcServer::disableAllTraffic()
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::disableAllTraffic";
+#endif
+
     return KillSwitch::instance()->disableAllTraffic();
 }
 
 bool IpcServer::enableKillSwitch(const QJsonObject &configStr, int vpnAdapterIndex)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::enableKillSwitch";
+#endif
+
     return KillSwitch::instance()->enableKillSwitch(configStr, vpnAdapterIndex);
 }
 
 bool IpcServer::disableKillSwitch()
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::disableKillSwitch";
+#endif
+
     return KillSwitch::instance()->disableKillSwitch();
 }
 
 bool IpcServer::enablePeerTraffic(const QJsonObject &configStr)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::enablePeerTraffic";
+#endif
+
     return KillSwitch::instance()->enablePeerTraffic(configStr);
 }
 
 bool IpcServer::refreshKillSwitch(bool enabled)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::refreshKillSwitch";
+#endif
+
     return KillSwitch::instance()->refresh(enabled);
 }
 
 void IpcServer::xrayStart(const QString& cfg)
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::xrayStart";
+#endif
+
     return Xray::getInstance().startXray(cfg);
 }
 
 void IpcServer::xrayStop()
 {
+#ifdef MZ_DEBUG
+    qDebug() << "IpcServer::xrayStop";
+#endif
+
     return Xray::getInstance().stopXray();
-}
+}