From 6b0b582b91d3631bf27d4dbf2979f7624625753f Mon Sep 17 00:00:00 2001 From: Alejandro Pacheco Date: Sat, 31 Oct 2026 16:16:59 +0100 Subject: [PATCH 1/3] Build with POSIX threads so the library's locks and per-thread data work thread-private.h only maps _ppdMutexLock()/_ppdMutexUnlock() and the thread-local storage macros onto pthreads when HAVE_PTHREAD_H is defined, but configure never checked for , so every build used the "no threading" fallbacks: the mutexes guarding the string pool (string.c) and the message catalogs (libcups2.c) did nothing, and ppdGlobals() (ppd.c) returned one structure shared by all threads. Printer applications that call libppd from several threads at once (one per job or printer, as pappl-retrofit does) thus raced on the string pool and saw each other's ppdLastError(). Check for (defining HAVE_PTHREAD_H) and for the library that provides pthread_key_create(). ppdGlobals(), and with it ppdLastError() and ppdSetConformance(), is now per thread as in CUPS; single-threaded programs see no change. Add test_ppd_thread, which checks that ppdLastError() is kept per thread and that concurrent use of the string pool keeps its references right. --- Makefile.am | 15 +- configure.ac | 5 + ppd/test_ppd_thread.c | 323 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 341 insertions(+), 2 deletions(-) create mode 100644 ppd/test_ppd_thread.c diff --git a/Makefile.am b/Makefile.am index eed7edd6..d8f390a7 100644 --- a/Makefile.am +++ b/Makefile.am @@ -67,7 +67,8 @@ check_PROGRAMS = \ test_ppd_emit \ test_ppd_filter \ test_ppd_generator \ - test_ppd_load_profile + test_ppd_load_profile \ + test_ppd_thread TESTS = \ testppd \ test_ppd_localize \ @@ -81,7 +82,8 @@ TESTS = \ test_ppd_emit \ test_ppd_filter \ test_ppd_generator \ - test_ppd_load_profile + test_ppd_load_profile \ + test_ppd_thread libppd_la_SOURCES = \ ppd/ppd-attr.c \ @@ -267,6 +269,15 @@ test_ppd_load_profile_CFLAGS = \ $(CUPS_CFLAGS) \ $(LIBCUPSFILTERS_CFLAGS) +test_ppd_thread_SOURCES = ppd/test_ppd_thread.c +test_ppd_thread_LDADD = \ + libppd.la \ + $(CUPS_LIBS) +test_ppd_thread_CFLAGS = \ + -I$(srcdir)/ppd/ \ + -pthread \ + $(CUPS_CFLAGS) + EXTRA_DIST += \ $(pkgppdinclude_DATA) \ $(pkgppddefs_DATA) \ diff --git a/configure.ac b/configure.ac index 8a1a895c..bfb3dd3f 100644 --- a/configure.ac +++ b/configure.ac @@ -257,6 +257,11 @@ AC_CHECK_HEADERS([dirent.h]) AC_CHECK_HEADER(string.h,AC_DEFINE(HAVE_STRING_H)) AC_CHECK_HEADER(strings.h,AC_DEFINE(HAVE_STRINGS_H)) +# =================================================================== +# Check for POSIX threads (mutexes and per-thread data of the library) +# =================================================================== +AC_CHECK_HEADERS([pthread.h], [AC_SEARCH_LIBS([pthread_key_create], [pthread])]) + # =================================== # Check for large files and long long # =================================== diff --git a/ppd/test_ppd_thread.c b/ppd/test_ppd_thread.c new file mode 100644 index 00000000..ae03f95c --- /dev/null +++ b/ppd/test_ppd_thread.c @@ -0,0 +1,323 @@ +// +// Thread-safety unit tests for libppd. +// +// Copyright © 2026 by OpenPrinting. +// +// Licensed under Apache License v2.0. See the file "LICENSE" for more +// information. +// +// Printer applications call libppd from several threads at once (one per +// job or printer), so state the library keeps between calls must be kept +// per thread or locked. Each group below checks one piece of that state. +// +// Tests covered: +// +// Group 1 ppdOpen() status (ppdLastError()): a ppdOpen() of a valid PPD +// in another thread leaves the error of a failed ppdOpen() in +// the main thread untouched. +// +// Group 2 String pool (_ppdStrAlloc(), _ppdStrRetain(), _ppdStrFree()): +// several threads allocate, retain and free a shared string and +// strings of their own in a loop; every string must read back +// right and the pool must hold as many references afterwards as +// before. +// +// +// The per-thread checks of group 1 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 into. +// + +#include +#include +#include "test-internal.h" +#include +#include +#include +#include + + +// +// Constants... +// + +#define NUM_THREADS 8 // Threads in the concurrent tests +#define NUM_LOOPS 2000 // Iterations per thread + + +// +// Local types... +// + +typedef struct thread_data_s // Per-thread test data +{ + int num; // Thread number + int loops, // Iterations done + mismatches; // Iterations with a wrong result +} thread_data_t; + + +// +// Minimal valid PPD for group 1. +// + +static const char test_ppd_text[] = + "*PPD-Adobe: \"4.3\"\n" + "*FormatVersion: \"4.3\"\n" + "*FileVersion: \"1.0\"\n" + "*LanguageVersion: English\n" + "*LanguageEncoding: ISOLatin1\n" + "*PCFileName: \"THREAD.PPD\"\n" + "*Product: \"(Test)\"\n" + "*ModelName: \"Thread Test Printer\"\n" + "*ShortNickName: \"ThreadTest\"\n" + "*NickName: \"Thread Test Printer\"\n" + "*PSVersion: \"(3010.000) 0\"\n" + "*LanguageLevel: \"3\"\n" + "*ColorDevice: False\n" + "*DefaultColorSpace: Gray\n" + "*FileSystem: False\n" + "*Throughput: \"1\"\n" + "*LandscapeOrientation: Plus90\n" + "*TTRasterizer: Type42\n"; + + +// +// Local functions... +// + +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_open_ppd(void *arg); +static void *run_string_pool(void *arg); +static int start_threads(pthread_t *threads, thread_data_t *data, + void *(*func)(void *)); +static void test_ppd_status(void); +static void test_string_pool(void); + + +// +// 'main()' - Run the thread-safety tests. +// + +int // O - Exit status +main(void) +{ + test_ppd_status(); + test_string_pool(); + + return (testsPassed ? 0 : 1); +} + + +// +// 'join_threads()' - Wait for the threads and add up their results. +// + +static int // O - Mismatches seen +join_threads(pthread_t *threads, // I - Threads + int started, // I - Threads started + thread_data_t *data, // I - Per-thread test data + int *loops) // O - Iterations done +{ + int i, // Looping var + mismatches; // Mismatches seen + + + for (i = 0, *loops = 0, mismatches = 0; i < started; i ++) + { + pthread_join(threads[i], NULL); + *loops += data[i].loops; + mismatches += data[i].mismatches; + } + + return (mismatches); +} + + +// +// 'open_ppd()' - Open a PPD from a string. +// + +static ppd_file_t * // O - PPD file or NULL +open_ppd(const char *text) // I - PPD text +{ + FILE *fp; // Temporary file + ppd_file_t *ppd; // PPD file + + + if ((fp = tmpfile()) == NULL) + return (NULL); + + fputs(text, fp); + rewind(fp); + + ppd = ppdOpen(fp); + + fclose(fp); + + return (ppd); +} + + +// +// 'run_open_ppd()' - Open a valid PPD and check its status. +// + +static void * // O - Thread exit value +run_open_ppd(void *arg) // I - Thread data +{ + thread_data_t *data = (thread_data_t *)arg; + // Thread data + ppd_file_t *ppd; // PPD file + int line; // Line number + + + data->loops ++; + + if ((ppd = open_ppd(test_ppd_text)) == NULL || + ppdLastError(&line) != PPD_OK) + data->mismatches ++; + + ppdClose(ppd); + + return (NULL); +} + + +// +// 'run_string_pool()' - Allocate, retain and free pooled strings in a loop. +// + +static void * // O - Thread exit value +run_string_pool(void *arg) // I - Thread data +{ + thread_data_t *data = (thread_data_t *)arg; + // Thread data + char own[64]; // String of this thread + char *shared, // Pooled shared string + *mine, // Pooled string of this thread + *retained; // Retained reference to it + + + for (data->loops = 0; data->loops < NUM_LOOPS; data->loops ++) + { + snprintf(own, sizeof(own), "libppd thread %d string %d", data->num, + data->loops % 16); + + shared = _ppdStrAlloc("libppd thread test shared string"); + mine = _ppdStrAlloc(own); + retained = _ppdStrRetain(mine); + + if (!shared || strcmp(shared, "libppd thread test shared string") || + !mine || strcmp(mine, own) || retained != mine) + data->mismatches ++; + + _ppdStrFree(retained); + _ppdStrFree(mine); + _ppdStrFree(shared); + } + + return (NULL); +} + + +// +// 'start_threads()' - Start one thread per test data record. +// + +static int // O - Threads started +start_threads(pthread_t *threads, // I - Threads + thread_data_t *data, // I - Per-thread test data + void *(*func)(void *)) + // I - Thread function +{ + int started; // Threads started + + + for (started = 0; started < NUM_THREADS; started ++) + if (pthread_create(threads + started, NULL, func, data + started)) + break; + + return (started); +} + + +// +// 'test_ppd_status()' - Test the status of ppdOpen() (group 1). +// + +static void +test_ppd_status(void) +{ + pthread_t thread; // Thread + thread_data_t data; // Thread data + ppd_file_t *ppd; // PPD file + ppd_status_t status, // Status of failed ppdOpen() + current; // Status now + int line, // Line of failed ppdOpen() + current_line; // Line now + + + testBegin("ppdOpen(not a PPD file) sets ppdLastError()"); + ppd = open_ppd("This is not a PPD file.\n"); + status = ppdLastError(&line); + testEndMessage(!ppd && status != PPD_OK, "status=%d (%s), line=%d", + status, ppdErrorString(status), line); + ppdClose(ppd); + + memset(&data, 0, sizeof(data)); + + testBegin("ppdOpen(valid PPD) in a new thread"); + if (pthread_create(&thread, NULL, run_open_ppd, &data)) + { + testEndMessage(false, "unable to create thread"); + } + else + { + pthread_join(thread, NULL); + testEnd(data.mismatches == 0); + } + + testBegin("ppdLastError() unchanged by the other thread"); + current = ppdLastError(¤t_line); + testEndMessage(current == status && current_line == line, + "status=%d (%s), line=%d", current, ppdErrorString(current), + current_line); +} + + +// +// 'test_string_pool()' - Test the string pool (group 2). +// + +static void +test_string_pool(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 + size_t before, // Strings in the pool before + after; // Strings in the pool after + + + memset(data, 0, sizeof(data)); + for (i = 0; i < NUM_THREADS; i ++) + data[i].num = i; + + before = _ppdStrStatistics(NULL, NULL); + + testBegin("_ppdStrAlloc/_ppdStrRetain/_ppdStrFree in %d threads x %d loops", + NUM_THREADS, NUM_LOOPS); + started = start_threads(threads, data, run_string_pool); + mismatches = join_threads(threads, started, data, &loops); + after = _ppdStrStatistics(NULL, NULL); + testEndMessage(started == NUM_THREADS && mismatches == 0 && after == before, + "%d threads, %d of %d strings wrong, %u references in the " + "pool before and %u after", started, mismatches, loops, + (unsigned)before, (unsigned)after); +} From e8308060ad708f911849116998a07bcce4b0f890 Mon Sep 17 00:00:00 2001 From: Alejandro Pacheco Date: Sat, 31 Oct 2026 16:16:59 +0100 Subject: [PATCH 2/3] Keep the raster error buffer per thread _ppdRasterAddError(), _ppdRasterClearError() and _ppdRasterErrorString() shared one static buffer (CUPS keeps it per thread), so concurrent ppdRasterInterpretPPD() and ppdRasterExecPS() calls, which clear the buffer and append errors to it, raced: one thread could realloc() the buffer while another copied a message into it, or append past its end after the other moved the write position. The buffer was also only allocated by _ppdRasterClearError(), so ppdRasterExecPS() reporting an error before any ppdRasterInterpretPPD() call dereferenced a NULL pointer. Allocate the buffer of each thread on first use and free it when the thread exits, with a thread key as ppdGlobals() uses (a plain static buffer when built without pthreads). Extend test_ppd_thread with the raster error checks. --- ppd/raster-error.c | 117 ++++++++++++++++-- ppd/test_ppd_thread.c | 278 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 382 insertions(+), 13 deletions(-) diff --git a/ppd/raster-error.c b/ppd/raster-error.c index 36d7c21f..5ba54191 100644 --- a/ppd/raster-error.c +++ b/ppd/raster-error.c @@ -13,6 +13,7 @@ // #include "raster-private.h" +#include "thread-private.h" #include "debug-internal.h" typedef struct _ppd_raster_error_s // **** Error buffer structure **** @@ -22,7 +23,32 @@ typedef struct _ppd_raster_error_s // **** Error buffer structure **** *end; // End of buffer } _ppd_raster_error_t; -static _ppd_raster_error_t *buf = NULL; + +// +// Local globals... +// + +#ifdef HAVE_PTHREAD_H +static _ppd_threadkey_t ppd_raster_error_key = _PPD_THREADKEY_INITIALIZER; + // Thread local storage key +static pthread_once_t ppd_raster_error_key_once = PTHREAD_ONCE_INIT; + // One-time initialization object +#else +static _ppd_raster_error_t ppd_raster_error_buf = { NULL, NULL, NULL }; + // Error buffer +#endif // HAVE_PTHREAD_H + + +// +// Local functions... +// + +static _ppd_raster_error_t *ppd_raster_error_buffer(void); +#ifdef HAVE_PTHREAD_H +static void ppd_raster_error_free(void *data); +static void ppd_raster_error_init(void); +#endif // HAVE_PTHREAD_H + // // '_ppdRasterAddError()' - Add an error message to the error buffer. @@ -32,6 +58,8 @@ void _ppdRasterAddError(const char *f, // I - Printf-style error message ...) // I - Additional arguments as needed { + _ppd_raster_error_t *buf = ppd_raster_error_buffer(); + // Error buffer for this thread va_list ap; // Pointer to additional arguments char s[2048]; // Message string ssize_t bytes; // Bytes in message string @@ -39,6 +67,9 @@ _ppdRasterAddError(const char *f, // I - Printf-style error message DEBUG_printf(("_ppdRasterAddError(f=\"%s\", ...)", f)); + if (!buf) + return; + va_start(ap, f); bytes = vsnprintf(s, sizeof(s), f, ap); va_end(ap); @@ -98,13 +129,12 @@ _ppdRasterAddError(const char *f, // I - Printf-style error message void _ppdRasterClearError(void) { - if (buf == NULL) - { - buf = malloc(sizeof(_ppd_raster_error_t)); - buf->start = NULL; - buf->end = NULL; - buf->current = NULL; - } + _ppd_raster_error_t *buf = ppd_raster_error_buffer(); + // Error buffer for this thread + + + if (!buf) + return; buf->current = buf->start; @@ -124,8 +154,77 @@ _ppdRasterClearError(void) const char * // O - Last error _ppdRasterErrorString(void) { - if (buf->current == buf->start) + _ppd_raster_error_t *buf = ppd_raster_error_buffer(); + // Error buffer for this thread + + + if (!buf || buf->current == buf->start) return (NULL); else return (buf->start); } + + +// +// 'ppd_raster_error_buffer()' - Return the error buffer of the current thread. +// + +static _ppd_raster_error_t * // O - Error buffer or NULL +ppd_raster_error_buffer(void) +{ +#ifdef HAVE_PTHREAD_H + _ppd_raster_error_t *buf; // Error buffer + + + // + // Initialize the thread key exactly once... + // + + pthread_once(&ppd_raster_error_key_once, ppd_raster_error_init); + + // + // Allocate the buffer of this thread on first use... + // + + if ((buf = (_ppd_raster_error_t *) + _ppdThreadGetData(ppd_raster_error_key)) == NULL) + { + if ((buf = calloc(1, sizeof(_ppd_raster_error_t))) != NULL) + _ppdThreadSetData(ppd_raster_error_key, buf); + } + + return (buf); + +#else + return (&ppd_raster_error_buf); +#endif // HAVE_PTHREAD_H +} + + +#ifdef HAVE_PTHREAD_H +// +// 'ppd_raster_error_free()' - Free the error buffer of a thread that exits. +// + +static void +ppd_raster_error_free(void *data) // I - Error buffer +{ + _ppd_raster_error_t *buf = (_ppd_raster_error_t *)data; + // Error buffer + + + free(buf->start); + free(buf); +} + + +// +// 'ppd_raster_error_init()' - Create the thread key for the error buffers. +// + +static void +ppd_raster_error_init(void) +{ + pthread_key_create(&ppd_raster_error_key, ppd_raster_error_free); +} +#endif // HAVE_PTHREAD_H diff --git a/ppd/test_ppd_thread.c b/ppd/test_ppd_thread.c index ae03f95c..0ba54604 100644 --- a/ppd/test_ppd_thread.c +++ b/ppd/test_ppd_thread.c @@ -22,13 +22,29 @@ // right and the pool must hold as many references afterwards as // before. // -// -// The per-thread checks of group 1 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 into. +// Group 3 Raster error buffer (_ppdRasterAddError(), _ppdRasterClearError() +// and _ppdRasterErrorString(), used by ppdRasterInterpretPPD() and +// ppdRasterExecPS()): +// - ppdRasterExecPS() as the first raster call of the process +// reports an unknown operator (-1 and its message) although no +// ppdRasterInterpretPPD() has cleared the buffer before, and +// ppdRasterInterpretPPD(NULL header) sets its error message; +// - a new thread starts with an empty buffer, and its errors +// leave the error message of the main thread untouched; +// - several threads run ppdRasterInterpretPPD() (which clears the +// buffer) and ppdRasterExecPS() (which adds an error for an +// unknown operator and one per object left on the stack) in a +// loop, each with its own code, and must read back exactly the +// message the same code gives when run alone. +// +// 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 +// into. // #include +#include #include #include "test-internal.h" #include @@ -52,6 +68,8 @@ typedef struct thread_data_s // Per-thread test data { int num; // Thread number + char code[4096]; // PostScript code to run + char *expected; // Error message the code gives alone int loops, // Iterations done mismatches; // Iterations with a wrong result } thread_data_t; @@ -90,10 +108,15 @@ 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_open_ppd(void *arg); +static void *run_raster_concurrent(void *arg); +static void *run_raster_new_thread(void *arg); static void *run_string_pool(void *arg); +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_ppd_status(void); +static void test_raster_error(void); static void test_string_pool(void); @@ -106,6 +129,7 @@ main(void) { test_ppd_status(); test_string_pool(); + test_raster_error(); return (testsPassed ? 0 : 1); } @@ -186,6 +210,84 @@ run_open_ppd(void *arg) // I - Thread data } +// +// 'run_raster_concurrent()' - Clear and add raster errors in a loop. +// + +static void * // O - Thread exit value +run_raster_concurrent(void *arg) // I - Thread data +{ + thread_data_t *data = (thread_data_t *)arg; + // Thread data + cups_page_header_t header; // Page header + int preferred_bits; // Preferred bits per color + const char *message; // Error message + + + for (data->loops = 0; data->loops < NUM_LOOPS; data->loops ++) + { + if (ppdRasterInterpretPPD(&header, NULL, 0, NULL, NULL) || + _ppdRasterErrorString() != NULL) + { + data->mismatches ++; + continue; + } + + preferred_bits = 0; + + if (ppdRasterExecPS(&header, &preferred_bits, data->code) != -1 || + (message = _ppdRasterErrorString()) == NULL || !data->expected || + strcmp(message, data->expected)) + data->mismatches ++; + } + + return (NULL); +} + + +// +// 'run_raster_new_thread()' - Add a raster error as a thread's first call. +// + +static void * // O - Thread exit value +run_raster_new_thread(void *arg) // I - Thread data +{ + thread_data_t *data = (thread_data_t *)arg; + // Thread data + cups_page_header_t header; // Page header + int preferred_bits; // Preferred bits per color + const char *message; // Error message + char expected[64]; // Start of the expected message + + + snprintf(expected, sizeof(expected), "Unknown operator \"bogus%d\"", + data->num); + + // The buffer of a new thread is empty... + data->loops ++; + if (_ppdRasterErrorString() != NULL) + data->mismatches ++; + + // ...and holds only this thread's error after ppdRasterExecPS() + memset(&header, 0, sizeof(header)); + preferred_bits = 0; + + data->loops ++; + if (ppdRasterExecPS(&header, &preferred_bits, data->code) != -1 || + (message = _ppdRasterErrorString()) == NULL || + strncmp(message, expected, strlen(expected))) + data->mismatches ++; + + // ppdRasterInterpretPPD() clears this thread's buffer + data->loops ++; + if (ppdRasterInterpretPPD(&header, NULL, 0, NULL, NULL) || + _ppdRasterErrorString() != NULL) + data->mismatches ++; + + return (NULL); +} + + // // 'run_string_pool()' - Allocate, retain and free pooled strings in a loop. // @@ -223,6 +325,73 @@ run_string_pool(void *arg) // I - Thread data } +// +// 'set_code()' - Set a thread's PostScript code. +// +// The code leaves a name, a number and 40 strings on the stack before an +// unknown operator, so each run adds over 40 errors (about 2k of text) and +// grows the buffer. +// + +static void +set_code(thread_data_t *data, // I - Thread data + int num) // I - Thread number +{ + char filler[49], // String value + *ptr; // Pointer into code + int i; // Looping var + + + memset(filler, 'a' + num, sizeof(filler) - 1); + filler[sizeof(filler) - 1] = '\0'; + + data->num = num; + data->loops = 0; + data->mismatches = 0; + + snprintf(data->code, sizeof(data->code), "/Thread%d %d", num, num); + for (i = 0, ptr = data->code + strlen(data->code); i < 40; i ++) + { + snprintf(ptr, sizeof(data->code) - (size_t)(ptr - data->code), " (%s)", + filler); + ptr += strlen(ptr); + } + snprintf(ptr, sizeof(data->code) - (size_t)(ptr - data->code), " bogus%d", + num); +} + + +// +// 'set_expected()' - Run a thread's code alone and keep its error message. +// + +static int // O - 1 on success, 0 on failure +set_expected(thread_data_t *data) // I - Thread data +{ + cups_page_header_t header; // Page header + int preferred_bits; // Preferred bits per color + const char *message; // Error message + char expected[64]; // Start of the expected message + + + snprintf(expected, sizeof(expected), "Unknown operator \"bogus%d\"", + data->num); + + ppdRasterInterpretPPD(&header, NULL, 0, NULL, NULL); + preferred_bits = 0; + + if (ppdRasterExecPS(&header, &preferred_bits, data->code) != -1 || + (message = _ppdRasterErrorString()) == NULL || + strncmp(message, expected, strlen(expected))) + return (0); + + free(data->expected); + data->expected = strdup(message); + + return (data->expected != NULL); +} + + // // 'start_threads()' - Start one thread per test data record. // @@ -288,6 +457,107 @@ test_ppd_status(void) } +// +// 'test_raster_error()' - Test the raster error buffer (group 3). +// + +static void +test_raster_error(void) +{ + cups_page_header_t header; // Page header + int preferred_bits; // Preferred bits per color + int status; // Function status + const char *message; // Error message + char *saved; // Copy of an error message + 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; // Wrong messages seen + + + // + // ppdRasterExecPS() before any ppdRasterInterpretPPD() (the groups before + // make no raster calls)... + // + + testBegin("ppdRasterExecPS(unknown operator) as the first call"); + memset(&header, 0, sizeof(header)); + preferred_bits = 0; + status = ppdRasterExecPS(&header, &preferred_bits, "bogus"); + message = _ppdRasterErrorString(); + testEndMessage(status == -1 && message && + !strncmp(message, "Unknown operator \"bogus\"", 24), + "status=%d, message=\"%s\"", status, + message ? message : "(null)"); + + testBegin("ppdRasterInterpretPPD(NULL header) sets an error"); + status = ppdRasterInterpretPPD(NULL, NULL, 0, NULL, NULL); + message = _ppdRasterErrorString(); + testEndMessage(status == -1 && message && + !strcmp(message, "Page header cannot be NULL!\n"), + "status=%d, message=\"%s\"", status, + message ? message : "(null)"); + + saved = strdup(message ? message : ""); + + // + // Errors are kept per thread... + // + + memset(data, 0, sizeof(data)); + set_code(data + 0, 0); + + testBegin("ppdRasterExecPS(unknown operator) in a new thread"); + if (pthread_create(threads + 0, NULL, run_raster_new_thread, data + 0)) + { + testEndMessage(false, "unable to create thread"); + } + else + { + pthread_join(threads[0], NULL); + testEndMessage(data[0].mismatches == 0, "%d of %d checks failed", + data[0].mismatches, data[0].loops); + } + + testBegin("_ppdRasterErrorString() unchanged by the other thread"); + message = _ppdRasterErrorString(); + testEndMessage(message && saved && !strcmp(message, saved), + "message=\"%s\"", message ? message : "(null)"); + + free(saved); + + // + // Concurrent clear/add in several threads... + // + + testBegin("Raster error messages of each code run alone"); + for (i = 0, mismatches = 0; i < NUM_THREADS; i ++) + { + set_code(data + i, i); + + if (!set_expected(data + i)) + mismatches ++; + } + testEndMessage(mismatches == 0, "%d of %d codes without the expected error", + mismatches, NUM_THREADS); + + testBegin("ppdRasterInterpretPPD/ppdRasterExecPS in %d threads x %d loops", + NUM_THREADS, NUM_LOOPS); + started = start_threads(threads, data, run_raster_concurrent); + mismatches = join_threads(threads, started, data, &loops); + testEndMessage(started == NUM_THREADS && mismatches == 0, + "%d threads, %d of %d messages wrong", started, mismatches, + loops); + + for (i = 0; i < NUM_THREADS; i ++) + free(data[i].expected); +} + + // // 'test_string_pool()' - Test the string pool (group 2). // From bab6dd6a951c4db9d3d6dbcbe9bf9c3b8068110d Mon Sep 17 00:00:00 2001 From: Alejandro Pacheco Date: Sat, 31 Oct 2026 16:16:59 +0100 Subject: [PATCH 3/3] 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. --- ppd/ppd-cache.c | 36 ++++++++++++++++--------- ppd/test_ppd_thread.c | 62 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 13 deletions(-) diff --git a/ppd/ppd-cache.c b/ppd/ppd-cache.c index be374423..d3b6eb1c 100644 --- a/ppd/ppd-cache.c +++ b/ppd/ppd-cache.c @@ -13,6 +13,7 @@ // #include +#include #include #include #include @@ -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); } // diff --git a/ppd/test_ppd_thread.c b/ppd/test_ppd_thread.c index 0ba54604..cb144bf0 100644 --- a/ppd/test_ppd_thread.c +++ b/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). //