sync: avoid setenv()setenv

set/getenv() are not thread-safe, and a recent bug report via private
email shows that this does cause segfaults:

Thread 4 (Thread 19251):
....

Thread 1 (Thread 19311):
...

In this case, DLT was used and the setenv call was setting the
LIBSYNTHESIS_<context> variables.

The solution is to avoid setenv() in code which might run in parallel
to other threads:
- DLT-related variables are set at the beginning of
  syncevo-dbus-server startup which then gets inherited
  by syncevo-dbus-helper and in the environment prepared
  for syncevo-local-sync (because the latter might run with
  a different log level)
- the default for SYNCEVOLUTION_PBAP_SYNC is now "incremental"
  everywhere and SyncContext is told about the special
  mode where it needs to keep photo data differently, i.e. setting
  SYNCEVOLUTION_PBAP_SYNC in dbus-sync.cpp for PBAP syncing is
  no longer necessary

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>

6 files changed, 81 insertions, 44 deletions

diff --git a/src/backends/pbap/PbapSyncSource.cpp b/src/backends/pbap/PbapSyncSource.cpp
index 15317d1f..4cf8200d 100644
--- a/src/backends/pbap/PbapSyncSource.cpp
+++ b/src/backends/pbap/PbapSyncSource.cpp
@@ -1140,7 +1140,7 @@ PbapSyncSource::PbapSyncSource(const SyncSourceParams &params) :
                                                this, _1, _2);
     m_session = PbapSession::create(*this);
     const char *PBAPSyncMode = getenv("SYNCEVOLUTION_PBAP_SYNC");
-    m_PBAPSyncMode = !PBAPSyncMode ? PBAP_SYNC_NORMAL :
+    m_PBAPSyncMode = !PBAPSyncMode ? PBAP_SYNC_INCREMENTAL :
         boost::iequals(PBAPSyncMode, "incremental") ? PBAP_SYNC_INCREMENTAL :
         boost::iequals(PBAPSyncMode, "text") ? PBAP_SYNC_TEXT :
         boost::iequals(PBAPSyncMode, "all") ? PBAP_SYNC_NORMAL :
diff --git a/src/dbus/server/dbus-sync.cpp b/src/dbus/server/dbus-sync.cpp
index 68994872..d0f92ce6 100644
--- a/src/dbus/server/dbus-sync.cpp
+++ b/src/dbus/server/dbus-sync.cpp
@@ -67,13 +67,7 @@ DBusSync::DBusSync(const SessionCommon::SyncParams &params,
             // "pbap" may only be used by caller when it knows that
             // the mode is safe to use.
             makeEphemeral();
-            const char *sync = getenv("SYNCEVOLUTION_PBAP_SYNC");
-            if (!sync) {
-                SE_LOG_DEBUG(NULL, "enabling default SYNCEVOLUTION_PBAP_SYNC=incremental");
-                setenv("SYNCEVOLUTION_PBAP_SYNC", "incremental", true);
-            } else {
-                SE_LOG_DEBUG(NULL, "using SYNCEVOLUTION_PBAP_SYNC=%s from environment", sync);
-            }
+            setKeepPhotoData(true);
         } else {
             filter["sync"] = params.m_mode;
         }
diff --git a/src/dbus/server/main.cpp b/src/dbus/server/main.cpp
index 8f54ec7b..60cec13d 100644
--- a/src/dbus/server/main.cpp
+++ b/src/dbus/server/main.cpp
@@ -146,6 +146,50 @@ int main(int argc, char **argv, char **envp)
             setenv("G_DBUS_DEBUG", gdbus, 1);
         }
 
+#ifdef USE_DLT
+        // set/getenv() are not thread-safe. We set them early to avoid
+        // conflicts with threads started by glib, because those tend
+        // to call getenv() randomly.
+        if (dltEnabled) {
+            // DLT logging with default log level DLT_LOG_WARN.  This
+            // default was chosen because DLT's own default,
+            // DLT_LOG_INFO, leads to too much output given that a lot
+            // of the standard messages in SyncEvolution and
+            // libsynthesis are labelled informational.
+            //
+            // SYNCEVOLUTION_USE_DLT and LIBSYNTHESIS_x for x standing for
+            // one of the libsynthesis context IDs (list see below) can
+            // also be set before invoking SyncEvolution, so here we only
+            // set them if unset.
+            std::string dlt_value = StringPrintf("%d", DLT_LOG_WARN);
+            setenv("SYNCEVOLUTION_USE_DLT", dlt_value.c_str(), false);
+            const char *contexts[] = {
+                "PROT",
+                "SESS",
+                "ADMN",
+                "DATA",
+                "REMI",
+                "PARS",
+                "GEN",
+                "TRNS",
+                "SMLT",
+                "SYS"
+            };
+            BOOST_FOREACH (const char *context, contexts) {
+                // Help libsynthesis debuglogger.cpp set default log levels,
+                // based on our own one.
+                SE_LOG_DEBUG(NULL, "default libsynthesis DLT logging of %s = %s",
+                             context, useDLT);
+                setenv((std::string("LIBSYNTHESIS_") + context).c_str(),
+                       dlt_value.c_str(),
+                       false);
+            }
+            loggerdlt.reset(new LoggerDLT(DLT_SYNCEVO_DBUS_SERVER_ID, "SyncEvolution D-Bus server"));
+        } else {
+            unsetenv("SYNCEVOLUTION_USE_DLT");
+        }
+#endif
+
         SyncContext::initMain(execName);
 
         loop = g_main_loop_new (NULL, FALSE);
@@ -158,17 +202,6 @@ int main(int argc, char **argv, char **envp)
         redirect->setLevel(stdoutEnabled ? level : Logger::NONE);
 #ifdef USE_DLT
         PushLogger<LoggerDLT> loggerdlt;
-        if (dltEnabled) {
-            // DLT logging with default log level DLT_LOG_WARN.  This
-            // default was chosen because DLT's own default,
-            // DLT_LOG_INFO, leads to too much output given that a lot
-            // of the standard messages in SyncEvolution and
-            // libsynthesis are labelled informational.
-            setenv("SYNCEVOLUTION_USE_DLT", StringPrintf("%d", DLT_LOG_WARN).c_str(), true);
-            loggerdlt.reset(new LoggerDLT(DLT_SYNCEVO_DBUS_SERVER_ID, "SyncEvolution D-Bus server"));
-        } else {
-            unsetenv("SYNCEVOLUTION_USE_DLT");
-        }
 #endif
         PushLogger<LoggerSyslog> syslogger;
         if (syslogEnabled && level > Logger::NONE) {
diff --git a/src/syncevo/LocalTransportAgent.cpp b/src/syncevo/LocalTransportAgent.cpp
index 6c26254a..7f660129 100644
--- a/src/syncevo/LocalTransportAgent.cpp
+++ b/src/syncevo/LocalTransportAgent.cpp
@@ -77,11 +77,18 @@ public:
         m_messageBufferSize = msgSize * 2;
         prepareBuffer(m_localBuffer, m_messageBufferSize);
         prepareBuffer(m_remoteBuffer, m_messageBufferSize);
-        setenv("SYNCEVOLUTION_LOCAL_SYNC_PARENT_FD", StringPrintf("%d", m_localBuffer.getFD()).c_str(), true);
-        setenv("SYNCEVOLUTION_LOCAL_SYNC_CHILD_FD", StringPrintf("%d", m_remoteBuffer.getFD()).c_str(), true);
         m_remoteBuffer.map(NULL, NULL);
     }
 
+    std::list<StringPair> getEnvForChild()
+    {
+        std::list<StringPair> env;
+
+        env.push_back(StringPair("SYNCEVOLUTION_LOCAL_SYNC_PARENT_FD", StringPrintf("%d", m_localBuffer.getFD())));
+        env.push_back(StringPair("SYNCEVOLUTION_LOCAL_SYNC_CHILD_FD", StringPrintf("%d", m_remoteBuffer.getFD())));
+        return env;
+    }
+
     void initChild(size_t msgSize)
     {
         m_messageBufferSize = msgSize * 2;
@@ -215,9 +222,29 @@ void LocalTransportAgent::start()
     m_forkexec = ForkExecParent::create("syncevo-local-sync");
 #ifdef USE_DLT
     if (getenv("SYNCEVOLUTION_USE_DLT")) {
-        m_forkexec->addEnvVar("SYNCEVOLUTION_USE_DLT", StringPrintf("%d", LoggerDLT::getCurrentDLTLogLevel()));
+        std::string dlt_value = StringPrintf("%d", LoggerDLT::getCurrentDLTLogLevel());
+        m_forkexec->addEnvVar("SYNCEVOLUTION_USE_DLT", dlt_value);
+        const char *contexts[] = {
+            "PROT",
+            "SESS",
+            "ADMN",
+            "DATA",
+            "REMI",
+            "PARS",
+            "GEN",
+            "TRNS",
+            "SMLT",
+            "SYS"
+        };
+        BOOST_FOREACH (const char *context, contexts) {
+            m_forkexec->addEnvVar(std::string("LIBSYNTHESIS_") + context
+                                  dlt_value);
+        }
     }
 #endif
+    BOOST_FOREACH(const StringPair &entry, SMLTKSharedMemory::singleton().getEnvForChild()) {
+        m_forkexec->addEnvVar(entry.first, entry.second);
+    }
     m_forkexec->m_onConnect.connect(boost::bind(&LocalTransportAgent::onChildConnect, this, _1));
     // fatal problems, including quitting child with non-zero status
     m_forkexec->m_onFailure.connect(boost::bind(&LocalTransportAgent::onFailure, this, _2));
diff --git a/src/syncevo/SyncContext.cpp b/src/syncevo/SyncContext.cpp
index 3bef32f3..98463dac 100644
--- a/src/syncevo/SyncContext.cpp
+++ b/src/syncevo/SyncContext.cpp
@@ -146,6 +146,7 @@ void SyncContext::init()
     m_doLogging = false;
     m_quiet = false;
     m_dryrun = false;
+    m_keepPhotoData = false;
     m_localSync = false;
     m_serverMode = false;
     m_serverAlerted = false;
@@ -2694,28 +2695,6 @@ void SyncContext::getConfigXML(bool isSync, string &xml, string &configname)
             "    <singlegloballog>yes</singlegloballog>\n";
 #ifdef USE_DLT
         if (useDLT) {
-            const char *contexts[] = {
-                "PROT",
-                "SESS",
-                "ADMN",
-                "DATA",
-                "REMI",
-                "PARS",
-                "GEN",
-                "TRNS",
-                "SMLT",
-                "SYS"
-            };
-            BOOST_FOREACH (const char *context, contexts) {
-                // Help libsynthesis debuglogger.cpp set default log levels,
-                // based on our own one.
-                SE_LOG_DEBUG(NULL, "default libsynthesis DLT logging of %s = %s",
-                             context, useDLT);
-                setenv((std::string("LIBSYNTHESIS_") + context).c_str(),
-                       useDLT,
-                       false);
-            }
-
             debug <<
                 // We have to enable all logging inside libsynthesis.
                 // The actual filtering then takes place inside DLT.
diff --git a/src/syncevo/SyncContext.h b/src/syncevo/SyncContext.h
index f14b8d5f..de9c73f0 100644
--- a/src/syncevo/SyncContext.h
+++ b/src/syncevo/SyncContext.h
@@ -64,6 +64,7 @@ class SyncContext : public SyncConfig {
     bool m_doLogging;
     bool m_quiet;
     bool m_dryrun;
+    bool m_keepPhotoData;
 
     enum SyncFreeze {
         SYNC_FREEZE_NONE,
@@ -225,6 +226,9 @@ class SyncContext : public SyncConfig {
     bool getDryRun() { return m_dryrun; }
     void setDryRun(bool dryrun) { m_dryrun = dryrun; }
 
+    bool getKeepPhotoData() const { return m_keepPhotoData; }
+    void setKeepPhotoData(bool keepPhotoData) { m_keepPhotoData = keepPhotoData; }
+
     bool isLocalSync() const { return m_localSync; }
 
     bool isServerAlerted() const { return m_serverAlerted; }