Re: Unsafe threading in syslogger on Windows - Mailing list pgsql-hackers

Andrew Dunstan wrote:
> Heikki Linnakangas wrote:
>> I'm going to see what happens if I remove all the #ifdef WIN32 blocks in
>> syslogger, and let it use pgpipe() and select() instead of the extra
>> thread.
>
> Sounds reasonable. Let's see how big the changes are on HEAD. I'm not
> sure it's worth creating a different smaller fix for the back branches.

I tried that, and got a crash somewhere in the code that inherits the
syslogger pipe/socket to the child process. I don't understand why, and
I don't feel like debugging any deeper into that right now. If you or
someone else wants to give it a shot, that would be good. If not, I
might try again some other day after sleeping over it.

Anyway, here's the patch I had in mind for back-branches.

--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 22cf5f2..ee449ac 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -117,7 +117,7 @@ HANDLE        syslogPipe[2] = {0, 0};

 #ifdef WIN32
 static HANDLE threadHandle = 0;
-static CRITICAL_SECTION sysfileSection;
+static CRITICAL_SECTION sysloggerSection;
 #endif

 /*
@@ -268,7 +268,8 @@ SysLoggerMain(int argc, char *argv[])

 #ifdef WIN32
     /* Fire up separate data transfer thread */
-    InitializeCriticalSection(&sysfileSection);
+    InitializeCriticalSection(&sysloggerSection);
+    EnterCriticalSection(&sysloggerSection);

     threadHandle = (HANDLE) _beginthreadex(NULL, 0, pipeThread, NULL, 0, NULL);
     if (threadHandle == 0)
@@ -423,8 +424,16 @@ SysLoggerMain(int argc, char *argv[])
          * On Windows we leave it to a separate thread to transfer data and
          * detect pipe EOF.  The main thread just wakes up once a second to
          * check for SIGHUP and rotation conditions.
+         *
+         * Server code isn't generally thread-safe, so we ensure that only
+         * one of the threads is active at a time by entering the critical
+         * section whenever we're not sleeping.
          */
+        LeaveCriticalSection(&sysloggerSection);
+
         pg_usleep(1000000L);
+
+        EnterCriticalSection(&sysloggerSection);
 #endif   /* WIN32 */

         if (pipe_eof_seen)
@@ -911,17 +920,9 @@ write_syslogger_file(const char *buffer, int count, int destination)
     if (destination == LOG_DESTINATION_CSVLOG && csvlogFile == NULL)
         open_csvlogfile();

-#ifdef WIN32
-    EnterCriticalSection(&sysfileSection);
-#endif
-
     logfile = destination == LOG_DESTINATION_CSVLOG ? csvlogFile : syslogFile;
     rc = fwrite(buffer, 1, count, logfile);

-#ifdef WIN32
-    LeaveCriticalSection(&sysfileSection);
-#endif
-
     /* can't use ereport here because of possible recursion */
     if (rc != count)
         write_stderr("could not write to log file: %s\n", strerror(errno));
@@ -945,11 +946,21 @@ pipeThread(void *arg)
     for (;;)
     {
         DWORD        bytesRead;
+        BOOL        result;
+
+        result = ReadFile(syslogPipe[0],
+                          logbuffer + bytes_in_logbuffer,
+                          sizeof(logbuffer) - bytes_in_logbuffer,
+                          &bytesRead, 0);

-        if (!ReadFile(syslogPipe[0],
-                      logbuffer + bytes_in_logbuffer,
-                      sizeof(logbuffer) - bytes_in_logbuffer,
-                      &bytesRead, 0))
+        /*
+         * Enter critical section before doing anything that might touch
+         * global state shared by the main thread. Anything that uses
+         * palloc()/pfree() in particular are not safe outside the critical
+         * section.
+         */
+        EnterCriticalSection(&sysloggerSection);
+        if (!result)
         {
             DWORD        error = GetLastError();

@@ -966,6 +977,7 @@ pipeThread(void *arg)
             bytes_in_logbuffer += bytesRead;
             process_pipe_input(logbuffer, &bytes_in_logbuffer);
         }
+        LeaveCriticalSection(&sysloggerSection);
     }

     /* We exit the above loop only upon detecting pipe EOF */
@@ -974,6 +986,7 @@ pipeThread(void *arg)
     /* if there's any data left then force it out now */
     flush_pipe_input(logbuffer, &bytes_in_logbuffer);

+    LeaveCriticalSection(&sysloggerSection);
     _endthread();
     return 0;
 }
@@ -1097,18 +1110,9 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
         _setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
 #endif

-        /* On Windows, need to interlock against data-transfer thread */
-#ifdef WIN32
-        EnterCriticalSection(&sysfileSection);
-#endif
-
         fclose(syslogFile);
         syslogFile = fh;

-#ifdef WIN32
-        LeaveCriticalSection(&sysfileSection);
-#endif
-
         /* instead of pfree'ing filename, remember it for next time */
         if (last_file_name != NULL)
             pfree(last_file_name);
@@ -1164,18 +1168,9 @@ logfile_rotate(bool time_based_rotation, int size_rotation_for)
         _setmode(_fileno(fh), _O_TEXT); /* use CRLF line endings on Windows */
 #endif

-        /* On Windows, need to interlock against data-transfer thread */
-#ifdef WIN32
-        EnterCriticalSection(&sysfileSection);
-#endif
-
         fclose(csvlogFile);
         csvlogFile = fh;

-#ifdef WIN32
-        LeaveCriticalSection(&sysfileSection);
-#endif
-
         /* instead of pfree'ing filename, remember it for next time */
         if (last_csv_file_name != NULL)
             pfree(last_csv_file_name);