Fixed shared files deadlock in a multi-threaded Windows application

- The shared files Windows implementation introduced in PR owasp-modsecurity#3132 works
  in multi-process single-threaded contexts but it doesn't work
  correctly in single-process multi-threaded contexts.
- The issue is that the LockFileEx Win32 function works on a per-handle
  basis.
  - In a multi-process context, each process will have called
    SharedFiles::add_new_handler when initializing the SharedFile and
    obtained a handle, and thus locking will work.
  - When running ModSecurity in a single process using multiple threads,
    the initialization of the SharedFile will happen once and the handle
    will be shared by all threads. Then, if two threads try to write to
    the same shared file concurrently, they may deadlock as one of them
    will lock the file (by calling LockFileEx) and then proceed to write
    to the file. If before writing to the file and unlocking it, another
    thread calls LockFileEx on the same handle, the attempt to write to
    the file will lock generating a deadlock.
- The new implementation replaces usage of LockFileEx/UnlockFileEx with
  a named mutex to lock access to the shared file.
  - A named mutex is used to support multi-process scenarios.
  - The mutex name is generated using the filename to support multiple
    shared files (such as that for the debug and audit logs).
    - This assumes that both process will initialize the SharedFile
      instance using the same filename (which is expected as they'd be
      using the same configuration file)

Author: eduar-hte

src/utils/shared_files.cc

@@ -17,8 +17,7 @@
 
 #include <fcntl.h>
 #ifdef WIN32
-#include <windows.h>
-#include <io.h>
+#include <algorithm>
 #endif
 
 
@@ -34,7 +33,32 @@ SharedFiles::handlers_map::iterator SharedFiles::add_new_handler(
         return m_handlers.end();
     }
 
-    return m_handlers.insert({ fileName, {fp, 0} }).first;
+#ifdef WIN32
+    // replace invalid characters for a Win32 named object
+    auto tmp = fileName;
+    std::replace(tmp.begin(), tmp.end(), '\\', '_');
+    std::replace(tmp.begin(), tmp.end(), '/', '_');
+
+    // use named mutex for multi-process locking support
+    const auto mutexName = "Global\\ModSecurity_" + tmp;
+
+    HANDLE hMutex = CreateMutex(NULL, FALSE, mutexName.c_str());
+    if (hMutex == NULL) {
+        error->assign("Failed to create mutex for shared file: " + fileName);
+        fclose(fp);
+        return m_handlers.end();
+    }
+#endif
+
+    auto handler = handler_info {
+        fp,
+#ifdef WIN32
+        hMutex,
+#endif
+        0
+    };
+    // cppcheck-suppress resourceLeak ; false positive, fp is closed in SharedFiles::close
+    return m_handlers.insert({ fileName, handler }).first;
 }
 
 
@@ -69,6 +93,9 @@ void SharedFiles::close(const std::string& fileName) {
     if (it->second.cnt == 0)
     {
         fclose(it->second.fp);
+#ifdef WIN32
+        CloseHandle(it->second.hMutex);
+#endif
 
         m_handlers.erase(it);
     }
@@ -92,9 +119,11 @@ bool SharedFiles::write(const std::string& fileName,
     lock.l_type = F_WRLCK;
     fcntl(fileno(it->second.fp), F_SETLKW, &lock);
 #else
-    auto handle = reinterpret_cast<HANDLE>(_get_osfhandle(fileno(it->second.fp)));
-    OVERLAPPED overlapped = { 0 };
-    ::LockFileEx(handle, LOCKFILE_EXCLUSIVE_LOCK, 0, MAXDWORD, MAXDWORD, &overlapped);
+    DWORD dwWaitResult = WaitForSingleObject(it->second.hMutex, INFINITE);
+    if (dwWaitResult != WAIT_OBJECT_0) {
+        error->assign("couldn't lock shared file: " + fileName);
+        return false;
+    }
 #endif
 
     auto wrote = fwrite(msg.c_str(), 1, msg.size(), it->second.fp);
@@ -109,8 +138,7 @@ bool SharedFiles::write(const std::string& fileName,
     lock.l_type = F_UNLCK;
     fcntl(fileno(it->second.fp), F_SETLKW, &lock);
 #else
-    overlapped = { 0 };
-    ::UnlockFileEx(handle, 0, MAXDWORD, MAXDWORD, &overlapped);
+    ::ReleaseMutex(it->second.hMutex);
 #endif
 
     return ret;

src/utils/shared_files.h

@@ -18,6 +18,9 @@
 
 
 #include <stdio.h>
+#ifdef WIN32
+#include <Windows.h>
+#endif
 
 #include <unordered_map>
 #include <string>
@@ -53,6 +56,9 @@ class SharedFiles {
 
      struct handler_info {
          FILE* fp;
+#ifdef WIN32
+         HANDLE hMutex;
+#endif
          unsigned int cnt;
      };