RemoteStore::addToStore(): Fix race between stderrThread and NAR writer

As pointed out by @B4dM4n, the call to to.flush() on stderrThread is
unsafe because the NAR writer thread is also writing to 'to'.

Fixes #3943.

3 files changed, 13 insertions, 8 deletions

diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index adba17c5e..e4a4ef5af 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -284,9 +284,9 @@ struct ConnectionHandle
 
     RemoteStore::Connection * operator -> () { return &*handle; }
 
-    void processStderr(Sink * sink = 0, Source * source = 0)
+    void processStderr(Sink * sink = 0, Source * source = 0, bool flush = true)
     {
-        auto ex = handle->processStderr(sink, source);
+        auto ex = handle->processStderr(sink, source, flush);
         if (ex) {
             daemonException = true;
             std::rethrow_exception(ex);
@@ -535,6 +535,8 @@ void RemoteStore::addToStore(const ValidPathInfo & info, Source & source,
 
         if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 23) {
 
+            conn->to.flush();
+
             std::exception_ptr ex;
 
             struct FramedSink : BufferedSink
@@ -574,7 +576,7 @@ void RemoteStore::addToStore(const ValidPathInfo & info, Source & source,
             std::thread stderrThread([&]()
             {
                 try {
-                    conn.processStderr();
+                    conn.processStderr(nullptr, nullptr, false);
                 } catch (...) {
                     ex = std::current_exception();
                 }
@@ -884,9 +886,10 @@ static Logger::Fields readFields(Source & from)
 }
 
 
-std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source * source)
+std::exception_ptr RemoteStore::Connection::processStderr(Sink * sink, Source * source, bool flush)
 {
-    to.flush();
+    if (flush)
+        to.flush();
 
     while (true) {
 
diff --git a/src/libstore/remote-store.hh b/src/libstore/remote-store.hh
index b319e774b..7cf4c4d12 100644
--- a/src/libstore/remote-store.hh
+++ b/src/libstore/remote-store.hh
@@ -114,7 +114,7 @@ protected:
 
         virtual ~Connection();
 
-        std::exception_ptr processStderr(Sink * sink = 0, Source * source = 0);
+        std::exception_ptr processStderr(Sink * sink = 0, Source * source = 0, bool flush = true);
     };
 
     ref<Connection> openConnectionWrapper();
diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh
index 69ae0874a..8f17bc34c 100644
--- a/src/libutil/serialise.hh
+++ b/src/libutil/serialise.hh
@@ -23,7 +23,8 @@ struct Sink
 };
 
 
-/* A buffered abstract sink. */
+/* A buffered abstract sink. Warning: a BufferedSink should not be
+   used from multiple threads concurrently. */
 struct BufferedSink : virtual Sink
 {
     size_t bufSize, bufPos;
@@ -66,7 +67,8 @@ struct Source
 };
 
 
-/* A buffered abstract source. */
+/* A buffered abstract source. Warning: a BufferedSink should not be
+   used from multiple threads concurrently. */
 struct BufferedSource : Source
 {
     size_t bufSize, bufPosIn, bufPosOut;