Fixed cppcheck-opensource#1650 (Cppcheck deadlock)

aggro80 · aggro80 · commit b5da0b8ed268 · 2010-07-07T15:42:39.000+03:00
http://sourceforge.net/apps/trac/cppcheck/ticket/1650
diff --git a/cli/threadexecutor.cpp b/cli/threadexecutor.cpp
@@ -27,6 +27,7 @@
 #include <cstdlib>
 #include <cstring>
 #include <cstdio>
+#include <errno.h>
 #endif
 
 ThreadExecutor::ThreadExecutor(const std::vector<std::string> &filenames, const Settings &settings, ErrorLogger &errorLogger)
@@ -51,12 +52,15 @@ void ThreadExecutor::addFileContent(const std::string &path, const std::string &
 
 #if (defined(__GNUC__) || defined(__sun)) && !defined(__MINGW32__)
 
-bool ThreadExecutor::handleRead(unsigned int &result)
+int ThreadExecutor::handleRead(unsigned int &result)
 {
     char type = 0;
     if (read(_pipe[0], &type, 1) <= 0)
     {
-        return false;
+        if( errno == EAGAIN )
+            return 0;
+
+        return -1;
     }
 
     if (type != '1' && type != '2' && type != '3')
@@ -104,10 +108,12 @@ bool ThreadExecutor::handleRead(unsigned int &result)
         iss >> fileResult;
         result += fileResult;
         _errorLogger.reportStatus(_fileCount, _filenames.size());
+        delete [] buf;
+        return -1;
     }
 
     delete [] buf;
-    return true;
+    return 1;
 }
 
 unsigned int ThreadExecutor::check()
@@ -134,65 +140,68 @@ unsigned int ThreadExecutor::check()
     }
 
     unsigned int childCount = 0;
-    for (unsigned int i = 0; i < _filenames.size(); i++)
+    unsigned int i = 0;
+    while( true )
     {
-        // Keep only wanted amount of child processes running at a time.
-        if (childCount >= _settings._jobs)
+        // Start a new child
+        if( i < _filenames.size() && childCount < _settings._jobs )
         {
-            while (handleRead(result))
+            pid_t pid = fork();
+            if (pid < 0)
+            {
+                // Error
+                std::cerr << "Failed to create child process" << std::endl;
+                exit(EXIT_FAILURE);
+            }
+            else if (pid == 0)
             {
+                CppCheck fileChecker(*this);
+                fileChecker.settings(_settings);
+
+                if (_fileContents.size() > 0 && _fileContents.find(_filenames[i]) != _fileContents.end())
+                {
+                    // File content was given as a string
+                    fileChecker.addFile(_filenames[i], _fileContents[ _filenames[i] ]);
+                }
+                else
+                {
+                    // Read file from a file
+                    fileChecker.addFile(_filenames[i]);
+                }
+
+                unsigned int resultOfCheck = fileChecker.check();
+                std::ostringstream oss;
+                oss << resultOfCheck;
+                writeToPipe('3', oss.str());
+                exit(0);
+            }
 
+            ++childCount;
+            ++i;
+        }
+        else if (childCount > 0)
+        {
+            // Wait for child to quit before stating new processes
+            while(true)
+            {
+                int readRes = handleRead(result);
+                if( readRes == -1 )
+                    break;
+                else if( readRes == 0 )
+                    usleep(5000); // 5 ms
             }
 
             int stat = 0;
             waitpid(0, &stat, 0);
             --childCount;
         }
-
-        pid_t pid = fork();
-        if (pid < 0)
+        else if(childCount == 0)
         {
-            // Error
-            std::cerr << "Failed to create child process" << std::endl;
-            exit(EXIT_FAILURE);
+            // All done
+            break;
         }
-        else if (pid == 0)
-        {
-            CppCheck fileChecker(*this);
-            fileChecker.settings(_settings);
-
-            if (_fileContents.size() > 0 && _fileContents.find(_filenames[i]) != _fileContents.end())
-            {
-                // File content was given as a string
-                fileChecker.addFile(_filenames[i], _fileContents[ _filenames[i] ]);
-            }
-            else
-            {
-                // Read file from a file
-                fileChecker.addFile(_filenames[i]);
-            }
-
-            unsigned int resultOfCheck = fileChecker.check();
-            std::ostringstream oss;
-            oss << resultOfCheck;
-            writeToPipe('3', oss.str());
-            exit(0);
-        }
-
-        ++childCount;
     }
 
-    while (childCount > 0)
-    {
-        int stat = 0;
-        waitpid(0, &stat, 0);
-        --childCount;
-    }
-
-    while (handleRead(result))
-    {
-
-    }
 
     return result;
 }
diff --git a/cli/threadexecutor.h b/cli/threadexecutor.h
@@ -58,7 +58,13 @@ class ThreadExecutor : public ErrorLogger
 
 #if (defined(__GNUC__) || defined(__sun)) && !defined(__MINGW32__)
 private:
-    bool handleRead(unsigned int &result);
+    /**
+     * Read from the pipe, parse and handle what ever is in there.
+     *@return -1 in case of error
+     *         0 if there is nothing in the pipe to be read
+     *         1 if we did read something
+     */
+    int handleRead(unsigned int &result);
     void writeToPipe(char type, const std::string &data);
     int _pipe[2];
     std::list<std::string> _errorList;
diff --git a/test/testthreadexecutor.cpp b/test/testthreadexecutor.cpp
@@ -74,8 +74,7 @@ class TestThreadExecutor : public TestFixture
 
     void run()
     {
-        // This is commented out, because it causes a deadlock
-        // TEST_CASE(deadlock_with_many_errors);
+        TEST_CASE(deadlock_with_many_errors);
         TEST_CASE(no_errors_more_files);
         TEST_CASE(no_errors_less_files);
         TEST_CASE(no_errors_equal_amount_files);

Original file line number	Diff line number	Diff line change
`@@ -74,8 +74,7 @@ class TestThreadExecutor : public TestFixture`
`74`	`74`
`75`	`75`	`void run()`
`76`	`76`	`{`
`77`		`- // This is commented out, because it causes a deadlock`
`78`		`- // TEST_CASE(deadlock_with_many_errors);`
	`77`	`+ TEST_CASE(deadlock_with_many_errors);`
`79`	`78`	`TEST_CASE(no_errors_more_files);`
`80`	`79`	`TEST_CASE(no_errors_less_files);`
`81`	`80`	`TEST_CASE(no_errors_equal_amount_files);`