D-Bus: fix syncevo-dbus-server<->syncevo-dbus-helper communication when using GIO D-Bus

D-Bus objects in a process exist independently from a specific D-Bus
connection. They are only identified by their path. When syncevo-dbus-server
forked multiple syncevo-dbus-helper instances, it indirectly (in ForkExec)
created multiple callback objects for the childs "watch" functionality (a
pending method call that the parent never replies to). These method calls were
associated with the a random (oldest?) session instead of the current one.

This causes problems once an older session terminates and the callback object
destructs, because then the wrong children get a failure response, which they
treat as "connection lost = parent has terminated". Found while trying to fix
shutdown race conditions.

The solution is to generate a unique counter for each child and communicate
that counter to the child via a new env variable.

4 files changed, 49 insertions, 12 deletions

diff --git a/src/dbus/server/session-helper.cpp b/src/dbus/server/session-helper.cpp
index 0df0004b..0ae993d9 100644
--- a/src/dbus/server/session-helper.cpp
+++ b/src/dbus/server/session-helper.cpp
@@ -107,7 +107,7 @@ SessionHelper::SessionHelper(GMainLoop *loop,
                              const boost::shared_ptr<ForkExecChild> &forkexec,
                              const boost::shared_ptr<LogRedirect> &parentLogger) :
     GDBusCXX::DBusObjectHelper(conn,
-                               SessionCommon::HELPER_PATH,
+                               std::string(SessionCommon::HELPER_PATH) + "/" + forkexec->getInstance(),
                                SessionCommon::HELPER_IFACE,
                                GDBusCXX::DBusObjectHelper::Callback_t(), // we don't care about a callback per message
                                true), // direct connection, close it when done
diff --git a/src/dbus/server/session.cpp b/src/dbus/server/session.cpp
index acae499c..134c8e17 100644
--- a/src/dbus/server/session.cpp
+++ b/src/dbus/server/session.cpp
@@ -43,9 +43,9 @@ SE_BEGIN_CXX
 class SessionProxy : public GDBusCXX::DBusRemoteObject
 {
 public:
-  SessionProxy(const GDBusCXX::DBusConnectionPtr &conn) :
+    SessionProxy(const GDBusCXX::DBusConnectionPtr &conn, const std::string &instance) :
     GDBusCXX::DBusRemoteObject(conn.get(),
-                               SessionCommon::HELPER_PATH,
+                               std::string(SessionCommon::HELPER_PATH) + "/" + instance,
                                SessionCommon::HELPER_IFACE,
                                SessionCommon::HELPER_DESTINATION,
                                true), // This is a one-to-one connection. Close it.
@@ -874,8 +874,9 @@ void Session::onConnect(const GDBusCXX::DBusConnectionPtr &conn) throw ()
 {
     PushLogger<Logger> guard(m_me);
     try {
-        SE_LOG_DEBUG(NULL, "helper has connected");
-        m_helper.reset(new SessionProxy(conn));
+        std::string instance = m_forkExecParent->getInstance();
+        SE_LOG_DEBUG(NULL, "helper %s has connected", instance.c_str());
+        m_helper.reset(new SessionProxy(conn, instance));
 
         // Activate signal watch on helper signals.
         m_helper->m_syncProgress.activate(boost::bind(&Session::syncProgress, this, _1, _2, _3, _4));
diff --git a/src/syncevo/ForkExec.cpp b/src/syncevo/ForkExec.cpp
index 46cd1d67..14ad4a6b 100644
--- a/src/syncevo/ForkExec.cpp
+++ b/src/syncevo/ForkExec.cpp
@@ -19,15 +19,18 @@
 
 #include "ForkExec.h"
 #include <syncevo/LogRedirect.h>
+#include <syncevo/ThreadSupport.h>
 
 #if defined(HAVE_GLIB)
 
 #include <pcrecpp.h>
+#include <ctype.h>
 #include "test.h"
 
 SE_BEGIN_CXX
 
 static const std::string ForkExecEnvVar("SYNCEVOLUTION_FORK_EXEC=");
+static const std::string ForkExecInstanceEnvVar("SYNCEVOLUTION_FORK_EXEC_INSTANCE=");
 
 #ifndef GDBUS_CXX_HAVE_DISCONNECT
 // internal D-Bus API: only used to monitor parent by having one method call pending
@@ -44,18 +47,34 @@ static const std::string FORKEXEC_PARENT_DESTINATION = "direct.peer"; // doesn't
 class ForkExecParentDBusAPI : public GDBusCXX::DBusObjectHelper
 {
 public:
-    ForkExecParentDBusAPI(const GDBusCXX::DBusConnectionPtr &conn) :
+    /**
+     * @param instance    a unique string to distinguish multiple different ForkExecParent
+     *                    instances; necessary because otherwise GIO GDBus may route messages from
+     *                    one connection to older instances on other connections
+     */
+    ForkExecParentDBusAPI(const GDBusCXX::DBusConnectionPtr &conn, const std::string &instance) :
         GDBusCXX::DBusObjectHelper(conn,
-                                   FORKEXEC_PARENT_PATH,
+                                   FORKEXEC_PARENT_PATH + "/" + instance,
                                    FORKEXEC_PARENT_IFACE)
     {
         add(this, &ForkExecParentDBusAPI::watch, "Watch");
         activate();
     }
 
+    ~ForkExecParentDBusAPI()
+    {
+        SE_LOG_DEBUG(NULL, "ForkExecParentDBusAPI %s: destroying with %ld active watches",
+                     getPath(),
+                     (long)m_watches.size());
+    }
+
+    bool hasWatches() const { return !m_watches.empty(); }
+
 private:
     void watch(const boost::shared_ptr< GDBusCXX::Result0> &result)
     {
+        SE_LOG_DEBUG(NULL, "ForkExecParentDBusAPI %s: received 'Watch' method call from child",
+                     getPath());
         m_watches.push_back(result);
     }
     std::list< boost::shared_ptr< GDBusCXX::Result0> > m_watches;
@@ -66,6 +85,9 @@ ForkExec::ForkExec()
 {
 }
 
+static Mutex ForkExecMutex;
+static unsigned int ForkExecCount;
+
 ForkExecParent::ForkExecParent(const std::string &helper) :
     m_helper(helper),
     m_childPid(0),
@@ -81,6 +103,9 @@ ForkExecParent::ForkExecParent(const std::string &helper) :
     m_errID(0),
     m_watchChild(NULL)
 {
+    Mutex::Guard guard = ForkExecMutex.lock();
+    ForkExecCount++;
+    m_instance = StringPrintf("forkexec%u", ForkExecCount);
 }
 
 boost::shared_ptr<ForkExecParent> ForkExecParent::create(const std::string &helper)
@@ -182,13 +207,15 @@ void ForkExecParent::start()
     for (char **env = environ;
          *env;
          env++) {
-        if (!boost::starts_with(*env, ForkExecEnvVar)) {
+        if (!boost::starts_with(*env, ForkExecEnvVar) &&
+            !boost::starts_with(*env, ForkExecInstanceEnvVar)) {
             m_envStrings.push_back(*env);
         }
     }
 
     // pass D-Bus address via env variable
     m_envStrings.push_back(ForkExecEnvVar + m_server->getAddress());
+    m_envStrings.push_back(ForkExecInstanceEnvVar + getInstance());
     m_env.reset(AllocStringArray(m_envStrings));
 
     SE_LOG_DEBUG(NULL, "ForkExecParent: running %s with D-Bus address %s",
@@ -375,7 +402,7 @@ void ForkExecParent::newClientConnection(GDBusCXX::DBusConnectionPtr &conn) thro
                      m_helper.c_str());
         m_hasConnected = true;
 #ifndef GDBUS_CXX_HAVE_DISCONNECT
-        m_api.reset(new ForkExecParentDBusAPI(conn));
+        m_api.reset(new ForkExecParentDBusAPI(conn, getInstance()));
 #endif
         m_onConnect(conn);
     } catch (...) {
@@ -442,6 +469,7 @@ void ForkExecParent::kill()
 ForkExecChild::ForkExecChild() :
     m_state(IDLE)
 {
+    m_instance = getEnv(ForkExecInstanceEnvVar.substr(0, ForkExecInstanceEnvVar.size() - 1).c_str(), "");
 }
 
 boost::shared_ptr<ForkExecChild> ForkExecChild::create()
@@ -480,16 +508,16 @@ void ForkExecChild::connect()
     class Parent : public GDBusCXX::DBusRemoteObject
     {
     public:
-        Parent(const GDBusCXX::DBusConnectionPtr &conn) :
+        Parent(const GDBusCXX::DBusConnectionPtr &conn, const std::string &instance) :
             GDBusCXX::DBusRemoteObject(conn,
-                                       FORKEXEC_PARENT_PATH,
+                                       FORKEXEC_PARENT_PATH + "/" + instance,
                                        FORKEXEC_PARENT_IFACE,
                                        FORKEXEC_PARENT_DESTINATION),
             m_watch(*this, "Watch")
         {}
 
         GDBusCXX::DBusClientCall0 m_watch;
-    } parent(conn);
+    } parent(conn, getInstance());
     parent.m_watch.start(boost::bind(&ForkExecChild::connectionLost, this));
 #endif
 
diff --git a/src/syncevo/ForkExec.h b/src/syncevo/ForkExec.h
index 5de1848c..e460e59d 100644
--- a/src/syncevo/ForkExec.h
+++ b/src/syncevo/ForkExec.h
@@ -100,8 +100,16 @@ class ForkExec : private boost::noncopyable {
     typedef boost::signals2::signal<void (SyncMLStatus, const std::string &)> OnFailure;
     OnFailure m_onFailure;
 
+    /**
+     * A unique string for the ForkExecParent/Child pair which can be used
+     * as D-Bus path component.
+     */
+    std::string getInstance() const { return m_instance; }
+
  protected:
     ForkExec();
+
+    std::string m_instance;
 };
 
 /**