From e37c7f589450373e439c842792eeb0d6d7ad80b7 Mon Sep 17 00:00:00 2001 From: Levi Neely <141506390+lneely@users.noreply.github.com> Date: Sat, 7 Mar 2026 19:01:17 +0100 Subject: [PATCH] Fix pcl-dls: errPtr memory leak between ptools_backend_call() calls in ptools_set_backend_file_dates() (#358) * Fix pcl-dls.1: free errPtr between calls in ptools_set_backend_file_dates() char *errPtr was already used instead of char msgErr[1024], but was not freed between the two ptools_backend_call() invocations. If the first call allocated errPtr, the second would overwrite the pointer without freeing it. Add free(errPtr); errPtr = NULL; between the two calls to eliminate the leak. Ref GH #194. Co-Authored-By: Claude Sonnet 4.6 * Add ptools_set_backend_file_dates() errPtr lifecycle tests (pcl-dls) 6 test cases using malloc/free wrapping covering both-succeed, call1-error, call2-error, both-errors, pre-fix leak demonstration, and no-double-free after mid-free NULLing. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Levi Neely Co-authored-by: Claude Sonnet 4.6 --- pclsync/ptools.c | 6 + tests/unit-tests/test_ptools_errptr.c | 252 ++++++++++++++++++++++++++ 2 files changed, 258 insertions(+) create mode 100644 tests/unit-tests/test_ptools_errptr.c diff --git a/pclsync/ptools.c b/pclsync/ptools.c index 947ef62..115021c 100644 --- a/pclsync/ptools.c +++ b/pclsync/ptools.c @@ -513,6 +513,12 @@ int ptools_set_backend_file_dates(uint64_t fileid, time_t ctime, time_t mtime) { pdbg_logf(D_NOTICE, "cTime res: [%d]", callRes); + /* Free errPtr before reuse to avoid leak if first call allocated it */ + if (errPtr) { + free(errPtr); + errPtr = NULL; + } + eventParams requiredParams = {5, {PAPI_STR("auth", psync_my_auth), PAPI_NUM("fileid", fileid), diff --git a/tests/unit-tests/test_ptools_errptr.c b/tests/unit-tests/test_ptools_errptr.c new file mode 100644 index 0000000..42edb7e --- /dev/null +++ b/tests/unit-tests/test_ptools_errptr.c @@ -0,0 +1,252 @@ +/* + * Test: ptools_set_backend_file_dates() errPtr freed between calls (pcl-dls) + * + * Verifies the fix in 11cecaa: + * - errPtr is freed (and NULLed) between the two ptools_backend_call() calls + * - errPtr is freed after the final call + * - No double-free occurs + * - NULL errPtr (no error on first call) is handled safely (no free of NULL) + * + * ptools_backend_call() needs a live backend so we replicate the exact + * errPtr management pattern with a mock backend call and use --wrap=malloc/free + * to track allocations. + */ + +#define _POSIX_C_SOURCE 200809L +#include +#include +#include +#include + +/* ------------------------------------------------------------------ */ +/* Allocation tracking via --wrap */ +/* ------------------------------------------------------------------ */ +int g_malloc_calls = 0; +int g_free_calls = 0; +void *g_freed[64]; +int g_freed_count = 0; +void *g_alloced[64]; +int g_alloced_count = 0; + +void *__real_malloc(size_t); +void __real_free(void *); + +void *__wrap_malloc(size_t sz) { + void *p = __real_malloc(sz); + if (p && g_alloced_count < 64) + g_alloced[g_alloced_count++] = p; + g_malloc_calls++; + return p; +} + +void __wrap_free(void *p) { + if (p && g_freed_count < 64) + g_freed[g_freed_count++] = p; + g_free_calls++; + __real_free(p); +} + +static void reset(void) { + g_malloc_calls = 0; + g_free_calls = 0; + g_freed_count = 0; + g_alloced_count = 0; + memset(g_freed, 0, sizeof(g_freed)); + memset(g_alloced, 0, sizeof(g_alloced)); +} + +static int was_freed(void *p) { + for (int i = 0; i < g_freed_count; i++) + if (g_freed[i] == p) return 1; + return 0; +} + +/* ------------------------------------------------------------------ */ +/* Mock backend call */ +/* Returns -1 and sets *errPtr when inject_error != 0. */ +/* ------------------------------------------------------------------ */ +static int inject_error_call1 = 0; +static int inject_error_call2 = 0; + +static int mock_backend_call_1(char **errPtr) { + if (inject_error_call1) { + *errPtr = (char *)malloc(32); + if (*errPtr) strcpy(*errPtr, "ctime error"); + return -1; + } + /* success: errPtr unchanged (stays NULL or previous value) */ + return 0; +} + +static int mock_backend_call_2(char **errPtr) { + if (inject_error_call2) { + *errPtr = (char *)malloc(32); + if (*errPtr) strcpy(*errPtr, "mtime error"); + return -1; + } + return 0; +} + +/* ------------------------------------------------------------------ */ +/* Replica of ptools_set_backend_file_dates() errPtr lifecycle (fixed) */ +/* ------------------------------------------------------------------ */ +static int run_fixed(void) { + char *errPtr = NULL; + int callRes; + + callRes = mock_backend_call_1(&errPtr); + (void)callRes; + + /* Fix: free errPtr between calls */ + if (errPtr) { + free(errPtr); + errPtr = NULL; + } + + callRes = mock_backend_call_2(&errPtr); + (void)callRes; + + if (errPtr) + free(errPtr); + + return 0; +} + +/* ------------------------------------------------------------------ */ +/* Replica of the OLD (unfixed) pattern — leaks errPtr between calls */ +/* ------------------------------------------------------------------ */ +static int run_unfixed(void) { + char *errPtr = NULL; + int callRes; + + callRes = mock_backend_call_1(&errPtr); + (void)callRes; + + /* BUG: errPtr NOT freed here — overwritten by call 2 */ + + callRes = mock_backend_call_2(&errPtr); + (void)callRes; + + if (errPtr) + free(errPtr); + + return 0; +} + +/* ------------------------------------------------------------------ */ +static int passes = 0, failures = 0; +#define PASS(n) do { printf("PASS: %s\n", n); passes++; } while(0) +#define FAIL(n, ...) do { printf("FAIL: %s — ", n); printf(__VA_ARGS__); printf("\n"); failures++; } while(0) + +/* ------------------------------------------------------------------ */ +/* Tests */ +/* ------------------------------------------------------------------ */ + +/* Both calls succeed: no allocations, no frees */ +static void test_both_succeed_no_leak(void) { + reset(); + inject_error_call1 = 0; + inject_error_call2 = 0; + run_fixed(); + if (g_malloc_calls == 0 && g_free_calls == 0) + PASS("both succeed: no malloc/free, no leak"); + else + FAIL("both succeed no leak", "malloc=%d free=%d", g_malloc_calls, g_free_calls); +} + +/* Call 1 sets errPtr, fixed code frees it before call 2 */ +static void test_call1_error_freed_before_call2(void) { + reset(); + inject_error_call1 = 1; + inject_error_call2 = 0; + run_fixed(); + + /* 1 malloc (call1), 1 free (between calls) */ + void *p = g_alloced_count > 0 ? g_alloced[0] : NULL; + if (g_malloc_calls == 1 && g_free_calls == 1 && p && was_freed(p)) + PASS("call1 error: errPtr freed before call2 (no leak)"); + else + FAIL("call1 error freed before call2", + "malloc=%d free=%d freed_ptr=%d", g_malloc_calls, g_free_calls, p ? was_freed(p) : -1); +} + +/* Call 2 sets errPtr, freed after final call */ +static void test_call2_error_freed_after(void) { + reset(); + inject_error_call1 = 0; + inject_error_call2 = 1; + run_fixed(); + + void *p = g_alloced_count > 0 ? g_alloced[0] : NULL; + if (g_malloc_calls == 1 && g_free_calls == 1 && p && was_freed(p)) + PASS("call2 error: errPtr freed after final call (no leak)"); + else + FAIL("call2 error freed after", + "malloc=%d free=%d", g_malloc_calls, g_free_calls); +} + +/* Both calls set errPtr: call1 ptr freed between, call2 ptr freed after */ +static void test_both_error_both_freed(void) { + reset(); + inject_error_call1 = 1; + inject_error_call2 = 1; + run_fixed(); + + /* 2 mallocs, 2 frees, both pointers freed */ + void *p1 = g_alloced_count > 0 ? g_alloced[0] : NULL; + void *p2 = g_alloced_count > 1 ? g_alloced[1] : NULL; + if (g_malloc_calls == 2 && g_free_calls == 2 + && p1 && was_freed(p1) && p2 && was_freed(p2)) + PASS("both errors: both errPtrs freed, no leak, no double-free"); + else + FAIL("both errors both freed", + "malloc=%d free=%d p1_freed=%d p2_freed=%d", + g_malloc_calls, g_free_calls, + p1 ? was_freed(p1) : -1, p2 ? was_freed(p2) : -1); +} + +/* Confirm the unfixed pattern LEAKS (documents what the fix corrects) */ +static void test_unfixed_leaks(void) { + reset(); + inject_error_call1 = 1; + inject_error_call2 = 1; + run_unfixed(); + + /* 2 mallocs, only 1 free — p1 is leaked */ + void *p1 = g_alloced_count > 0 ? g_alloced[0] : NULL; + void *p2 = g_alloced_count > 1 ? g_alloced[1] : NULL; + if (g_malloc_calls == 2 && g_free_calls == 1 + && p1 && !was_freed(p1) && p2 && was_freed(p2)) + PASS("(unfixed pattern confirmed): call1 errPtr leaks when not freed between calls"); + else + FAIL("unfixed pattern leak demonstration", + "malloc=%d free=%d p1_freed=%d", + g_malloc_calls, g_free_calls, p1 ? was_freed(p1) : -1); +} + +/* NULL errPtr after free: second call on NULL is safe (no double-free) */ +static void test_no_double_free(void) { + reset(); + inject_error_call1 = 1; + inject_error_call2 = 0; /* call2 succeeds, errPtr stays NULL after mid-free */ + run_fixed(); + + /* Only 1 malloc, 1 free */ + if (g_malloc_calls == 1 && g_free_calls == 1) + PASS("no double-free: errPtr NULLed after mid-free, call2 success leaves it NULL"); + else + FAIL("no double-free", "malloc=%d free=%d", g_malloc_calls, g_free_calls); +} + +/* ------------------------------------------------------------------ */ +int main(void) { + test_both_succeed_no_leak(); + test_call1_error_freed_before_call2(); + test_call2_error_freed_after(); + test_both_error_both_freed(); + test_unfixed_leaks(); + test_no_double_free(); + + printf("\n%d passed, %d failed\n", passes, failures); + return failures ? 1 : 0; +}