Plugin Scanning: Fix thread sanitizer issues in the AudioPluginHost

modules/juce_audio_processors/scanning/juce_PluginDirectoryScanner.h

@@ -126,7 +126,7 @@ private:
     File deadMansPedalFile;
     StringArray failedFiles;
     Atomic<int> nextIndex;
-    float progress = 0;
+    std::atomic<float> progress { 0.0f };
     const bool allowAsync;
 
     void updateProgress();

modules/juce_audio_processors/scanning/juce_PluginListComponent.cpp

@@ -451,7 +451,8 @@ private:
     String pluginBeingScanned;
     double progress = 0;
     const int numThreads;
-    bool allowAsync, finished = false, timerReentrancyCheck = false;
+    bool allowAsync, timerReentrancyCheck = false;
+    std::atomic<bool> finished { false };
     std::unique_ptr<ThreadPool> pool;
     std::set<String> initiallyBlacklistedFiles;
 
@@ -582,6 +583,8 @@ private:
         if (timerReentrancyCheck)
             return;
 
+        progress = scanner->getProgress();
+
         if (pool == nullptr)
         {
             const ScopedValueSetter<bool> setter (timerReentrancyCheck, true);
@@ -602,10 +605,7 @@ private:
     bool doNextScan()
     {
         if (scanner->scanNextFile (true, pluginBeingScanned))
-        {
-            progress = scanner->getProgress();
             return true;
-        }
 
         finished = true;
         return false;

modules/juce_core/files/juce_TemporaryFile.cpp

@@ -23,6 +23,23 @@
 namespace juce
 {
 
+// Using Random::getSystemRandom() can be a bit dangerous in multithreaded contexts!
+class LockedRandom
+{
+public:
+    int nextInt()
+    {
+        const ScopedLock lock (mutex);
+        return random.nextInt();
+    }
+
+private:
+    CriticalSection mutex;
+    Random random;
+};
+
+static LockedRandom lockedRandom;
+
 static File createTempFile (const File& parentDirectory, String name,
                             const String& suffix, int optionFlags)
 {
@@ -34,7 +51,7 @@ static File createTempFile (const File& parentDirectory, String name,
 
 TemporaryFile::TemporaryFile (const String& suffix, const int optionFlags)
     : temporaryFile (createTempFile (File::getSpecialLocation (File::tempDirectory),
-                                     "temp_" + String::toHexString (Random::getSystemRandom().nextInt()),
+                                     "temp_" + String::toHexString (lockedRandom.nextInt()),
                                      suffix, optionFlags)),
       targetFile()
 {
@@ -43,7 +60,7 @@ TemporaryFile::TemporaryFile (const String& suffix, const int optionFlags)
 TemporaryFile::TemporaryFile (const File& target, const int optionFlags)
     : temporaryFile (createTempFile (target.getParentDirectory(),
                                      target.getFileNameWithoutExtension()
-                                       + "_temp" + String::toHexString (Random::getSystemRandom().nextInt()),
+                                       + "_temp" + String::toHexString (lockedRandom.nextInt()),
                                      target.getFileExtension(), optionFlags)),
       targetFile (target)
 {

modules/juce_core/native/juce_posix_NamedPipe.cpp

@@ -39,8 +39,8 @@ public:
 
     ~Pimpl()
     {
-        if (pipeIn  != -1)  ::close (pipeIn);
-        if (pipeOut != -1)  ::close (pipeOut);
+        pipeIn .close();
+        pipeOut.close();
 
         if (createdPipe)
         {
@@ -51,7 +51,7 @@ public:
 
     bool connect (int timeOutMilliseconds)
     {
-        return openPipe (true, getTimeoutEnd (timeOutMilliseconds));
+        return openPipe (true, getTimeoutEnd (timeOutMilliseconds)) != invalidPipe;
     }
 
     int read (char* destBuffer, int maxBytesToRead, int timeOutMilliseconds)
@@ -61,8 +61,10 @@ public:
 
         while (bytesRead < maxBytesToRead)
         {
+            const auto pipe = pipeIn.get();
+
             auto bytesThisTime = maxBytesToRead - bytesRead;
-            auto numRead = (int) ::read (pipeIn, destBuffer, (size_t) bytesThisTime);
+            auto numRead = (int) ::read (pipe, destBuffer, (size_t) bytesThisTime);
 
             if (numRead <= 0)
             {
@@ -72,9 +74,9 @@ public:
                     return -1;
 
                 const int maxWaitingTime = 30;
-                waitForInput (pipeIn, timeoutEnd == 0 ? maxWaitingTime
-                                                      : jmin (maxWaitingTime,
-                                                              (int) (timeoutEnd - Time::getMillisecondCounter())));
+                waitForInput (pipe, timeoutEnd == 0 ? maxWaitingTime
+                                                    : jmin (maxWaitingTime,
+                                                            (int) (timeoutEnd - Time::getMillisecondCounter())));
                 continue;
             }
 
@@ -89,7 +91,9 @@ public:
     {
         auto timeoutEnd = getTimeoutEnd (timeOutMilliseconds);
 
-        if (! openPipe (false, timeoutEnd))
+        const auto pipe = openPipe (false, timeoutEnd);
+
+        if (pipe == invalidPipe)
             return -1;
 
         int bytesWritten = 0;
@@ -97,7 +101,7 @@ public:
         while (bytesWritten < numBytesToWrite && ! hasExpired (timeoutEnd))
         {
             auto bytesThisTime = numBytesToWrite - bytesWritten;
-            auto numWritten = (int) ::write (pipeOut, sourceBuffer, (size_t) bytesThisTime);
+            auto numWritten = (int) ::write (pipe, sourceBuffer, (size_t) bytesThisTime);
 
             if (numWritten < 0)
             {
@@ -105,9 +109,9 @@ public:
                 const int maxWaitingTime = 30;
 
                 if (error == EWOULDBLOCK || error == EAGAIN)
-                    waitToWrite (pipeOut, timeoutEnd == 0 ? maxWaitingTime
-                                                          : jmin (maxWaitingTime,
-                                                                  (int) (timeoutEnd - Time::getMillisecondCounter())));
+                    waitToWrite (pipe, timeoutEnd == 0 ? maxWaitingTime
+                                                       : jmin (maxWaitingTime,
+                                                               (int) (timeoutEnd - Time::getMillisecondCounter())));
                 else
                     return -1;
 
@@ -134,8 +138,52 @@ public:
         return createdFifoIn && createdFifoOut;
     }
 
+    static constexpr auto invalidPipe = -1;
+
+    class PipeDescriptor
+    {
+    public:
+        template <typename Fn>
+        int get (Fn&& fn)
+        {
+            {
+                const ScopedReadLock l (mutex);
+
+                if (descriptor != invalidPipe)
+                    return descriptor;
+            }
+
+            const ScopedWriteLock l (mutex);
+            return descriptor = fn();
+        }
+
+        void close()
+        {
+            {
+                const ScopedReadLock l (mutex);
+
+                if (descriptor == invalidPipe)
+                    return;
+            }
+
+            const ScopedWriteLock l (mutex);
+            ::close (descriptor);
+            descriptor = invalidPipe;
+        }
+
+        int get()
+        {
+            const ScopedReadLock l (mutex);
+            return descriptor;
+        }
+
+    private:
+        ReadWriteLock mutex;
+        int descriptor = invalidPipe;
+    };
+
     const String pipeInName, pipeOutName;
-    int pipeIn = -1, pipeOut = -1;
+    PipeDescriptor pipeIn, pipeOut;
     bool createdFifoIn = false, createdFifoOut = false;
 
     const bool createdPipe;
@@ -160,30 +208,25 @@ private:
         {
             auto p = ::open (name.toUTF8(), flags);
 
-            if (p != -1 || hasExpired (timeoutEnd) || stopReadOperation.load())
+            if (p != invalidPipe || hasExpired (timeoutEnd) || stopReadOperation.load())
                 return p;
 
             Thread::sleep (2);
         }
     }
 
-    bool openPipe (bool isInput, uint32 timeoutEnd)
+    int openPipe (bool isInput, uint32 timeoutEnd)
     {
         auto& pipe = isInput ? pipeIn : pipeOut;
-        int flags = (isInput ? O_RDWR : O_WRONLY) | O_NONBLOCK;
+        const auto flags = (isInput ? O_RDWR : O_WRONLY) | O_NONBLOCK;
 
         const String& pipeName = isInput ? (createdPipe ? pipeInName : pipeOutName)
                                          : (createdPipe ? pipeOutName : pipeInName);
 
-        if (pipe == -1)
+        return pipe.get ([this, &pipeName, &flags, &timeoutEnd]
         {
-            pipe = openPipe (pipeName, flags, timeoutEnd);
-
-            if (pipe == -1)
-                return false;
-        }
-
-        return true;
+            return openPipe (pipeName, flags, timeoutEnd);
+        });
     }
 
     static void waitForInput (int handle, int timeoutMsecs) noexcept
@@ -203,23 +246,18 @@ private:
 
 void NamedPipe::close()
 {
-    {
-        ScopedReadLock sl (lock);
+    ScopedWriteLock sl (lock);
 
-        if (pimpl != nullptr)
-        {
-            pimpl->stopReadOperation = true;
+    if (pimpl != nullptr)
+    {
+        pimpl->stopReadOperation = true;
 
-            char buffer[1] = { 0 };
-            ssize_t done = ::write (pimpl->pipeIn, buffer, 1);
-            ignoreUnused (done);
-        }
+        const char buffer[] { 0 };
+        const auto done = ::write (pimpl->pipeIn.get(), buffer, numElementsInArray (buffer));
+        ignoreUnused (done);
     }
 
-    {
-        ScopedWriteLock sl (lock);
-        pimpl.reset();
-    }
+    pimpl.reset();
 }
 
 bool NamedPipe::openInternal (const String& pipeName, bool createPipe, bool mustNotExist)

modules/juce_events/interprocess/juce_ConnectedChildProcess.cpp

@@ -51,6 +51,8 @@ struct ChildProcessPingThread  : public Thread,
         pingReceived();
     }
 
+    void startPinging()                     { startThread (4); }
+
     void pingReceived() noexcept            { countdown = timeoutMs / 1000 + 1; }
     void triggerConnectionLostMessage()     { triggerAsyncUpdate(); }
 
@@ -90,8 +92,7 @@ struct ChildProcessCoordinator::Connection  : public InterprocessConnection,
           ChildProcessPingThread (timeout),
           owner (m)
     {
-        if (createPipe (pipeName, timeoutMs))
-            startThread (4);
+        createPipe (pipeName, timeoutMs);
     }
 
     ~Connection() override
@@ -99,6 +100,8 @@ struct ChildProcessCoordinator::Connection  : public InterprocessConnection,
         stopThread (10000);
     }
 
+    using ChildProcessPingThread::startPinging;
+
 private:
     void connectionMade() override  {}
     void connectionLost() override  { owner.handleConnectionLost(); }
@@ -198,7 +201,6 @@ struct ChildProcessWorker::Connection  : public InterprocessConnection,
           owner (p)
     {
         connectToPipe (pipeName, timeoutMs);
-        startThread (4);
     }
 
     ~Connection() override
@@ -207,6 +209,8 @@ struct ChildProcessWorker::Connection  : public InterprocessConnection,
         disconnect();
     }
 
+    using ChildProcessPingThread::startPinging;
+
 private:
     ChildProcessWorker& owner;
 
@@ -275,7 +279,9 @@ bool ChildProcessWorker::initialiseFromCommandLine (const String& commandLine,
         {
             connection.reset (new Connection (*this, pipeName, timeoutMs <= 0 ? defaultTimeoutMs : timeoutMs));
 
-            if (! connection->isConnected())
+            if (connection->isConnected())
+                connection->startPinging();
+            else
                 connection.reset();
         }
     }