Swap the PPD cache status message under a lock

set_error() freed ppd_cache_status_message and then stored the new
message, so two threads failing in ppdCacheCreateWithFile() or
ppdCacheWriteFile() at the same time could both free the same pooled
string.  Build the new message first, swap it in under a mutex and free
the previous one afterwards.

Extend test_ppd_thread with concurrent ppdCacheCreateWithFile() failures.

Author: alepacheco

ppd/ppd-cache.c

@@ -13,6 +13,7 @@
 //
 
 #include <ppd/string-private.h>
+#include <ppd/thread-private.h>
 #include <ppd/array-private.h>
 #include <ppd/ipp-private.h>
 #include <ppd/ppd.h>
@@ -76,6 +77,8 @@ static int	ppd_pwg_compare_finishings(ppd_pwg_finishings_t *a,
 static void	ppd_pwg_free_finishings(ppd_pwg_finishings_t *f);
 
 char            *ppd_cache_status_message = NULL; // Last PPD cache error
+static _ppd_mutex_t	ppd_cache_status_mutex = _PPD_MUTEX_INITIALIZER;
+					// Mutex to control access to it
 
 
 //
@@ -86,19 +89,16 @@ static void
 set_error(const char   *message,	// I - status-message value
 	  int          localize)	// I - Localize the message?
 {
+  char	*new_message = NULL,		// New status-message
+	*old_message;			// Previous status-message
+
+
   if (!message && errno)
   {
     message  = strerror(errno);
     localize = 0;
   }
 
-  if (ppd_cache_status_message)
-  {
-    _ppdStrFree(ppd_cache_status_message);
-
-    ppd_cache_status_message = NULL;
-  }
-
   if (message)
   {
     if (localize)
@@ -107,16 +107,26 @@ set_error(const char   *message,	// I - status-message value
      // Get the message catalog...
      //
 
-      ppd_cache_status_message =
-	_ppdStrAlloc(cupsLangGetString(cupsLangDefault(),
-				      message));
+      new_message = _ppdStrAlloc(cupsLangGetString(cupsLangDefault(),
+						   message));
     }
     else
-      ppd_cache_status_message = _ppdStrAlloc(message);
+      new_message = _ppdStrAlloc(message);
   }
 
-  DEBUG_printf(("4set_error: last_status_message=\"%s\"",
-                ppd_cache_status_message));
+  DEBUG_printf(("4set_error: last_status_message=\"%s\"", new_message));
+
+  //
+  // Swap in the new message and free the previous one, so that threads
+  // setting errors at the same time never free the same string twice...
+  //
+
+  _ppdMutexLock(&ppd_cache_status_mutex);
+  old_message              = ppd_cache_status_message;
+  ppd_cache_status_message = new_message;
+  _ppdMutexUnlock(&ppd_cache_status_mutex);
+
+  _ppdStrFree(old_message);
 }
 
 //

ppd/test_ppd_thread.c

@@ -37,6 +37,10 @@
 //              loop, each with its own code, and must read back exactly the
 //              message the same code gives when run alone.
 //
+//   Group 4  PPD cache status message: several threads call
+//            ppdCacheCreateWithFile() on a file that does not exist in a
+//            loop, each failure replacing the last status message.
+//
 // The per-thread checks of groups 1 and 3 fail deterministically when the
 // state is shared by all threads.  The concurrent checks may crash or pass
 // by luck then; build with -fsanitize=thread to see every data race they run
@@ -107,6 +111,7 @@ static const char test_ppd_text[] =
 static int		join_threads(pthread_t *threads, int started,
 			             thread_data_t *data, int *loops);
 static ppd_file_t	*open_ppd(const char *text);
+static void		*run_cache_status(void *arg);
 static void		*run_open_ppd(void *arg);
 static void		*run_raster_concurrent(void *arg);
 static void		*run_raster_new_thread(void *arg);
@@ -115,6 +120,7 @@ static void		set_code(thread_data_t *data, int num);
 static int		set_expected(thread_data_t *data);
 static int		start_threads(pthread_t *threads, thread_data_t *data,
 			              void *(*func)(void *));
+static void		test_cache_status(void);
 static void		test_ppd_status(void);
 static void		test_raster_error(void);
 static void		test_string_pool(void);
@@ -130,6 +136,7 @@ main(void)
   test_ppd_status();
   test_string_pool();
   test_raster_error();
+  test_cache_status();
 
   return (testsPassed ? 0 : 1);
 }
@@ -185,6 +192,32 @@ open_ppd(const char *text)		// I - PPD text
 }
 
 
+//
+// 'run_cache_status()' - Fail to load a PPD cache file in a loop.
+//
+
+static void *				// O - Thread exit value
+run_cache_status(void *arg)		// I - Thread data
+{
+  thread_data_t	*data = (thread_data_t *)arg;
+					// Thread data
+  ppd_cache_t	*pc;			// PPD cache
+
+
+  for (data->loops = 0; data->loops < NUM_LOOPS; data->loops ++)
+  {
+    if ((pc = ppdCacheCreateWithFile("/nonexistent/libppd-test.cache",
+                                     NULL)) != NULL)
+    {
+      ppdCacheDestroy(pc);
+      data->mismatches ++;
+    }
+  }
+
+  return (NULL);
+}
+
+
 //
 // 'run_open_ppd()' - Open a valid PPD and check its status.
 //
@@ -413,6 +446,35 @@ start_threads(pthread_t     *threads,	// I - Threads
 }
 
 
+//
+// 'test_cache_status()' - Test the PPD cache status message (group 4).
+//
+
+static void
+test_cache_status(void)
+{
+  pthread_t	threads[NUM_THREADS];	// Threads
+  thread_data_t	data[NUM_THREADS];	// Per-thread test data
+  int		i,			// Looping var
+		started,		// Threads started
+		loops,			// Iterations done
+		mismatches;		// Mismatches seen
+
+
+  memset(data, 0, sizeof(data));
+  for (i = 0; i < NUM_THREADS; i ++)
+    data[i].num = i;
+
+  testBegin("ppdCacheCreateWithFile(missing file) in %d threads x %d loops",
+            NUM_THREADS, NUM_LOOPS);
+  started    = start_threads(threads, data, run_cache_status);
+  mismatches = join_threads(threads, started, data, &loops);
+  testEndMessage(started == NUM_THREADS && mismatches == 0,
+                 "%d threads, %d of %d calls did not fail", started,
+                 mismatches, loops);
+}
+
+
 //
 // 'test_ppd_status()' - Test the status of ppdOpen() (group 1).
 //