lkn
/
pcloudcc-lneely
mirror of https://github.com/lneely/pcloudcc-lneely.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Refactor test_ptask_free to link production code via --wrap 

Extract psync_task_free + psync_task_destroy (and their static helpers
psync_task_dec_refcnt, psync_task_entry) from ptask.c into a new
separately-compilable unit pclsync/ptask_free.c. Add
pclsync/ptask_free_internal.h to expose the internal struct layout
(struct psync_task_manager_t_ / struct psync_task_t_) for test use
without pulling in ptask.c's heavyweight transitive dependencies.

Rewrite tests/unit-tests/test_ptask_free.c to:
- Include ptask_free_internal.h instead of duplicating structs inline
- Call the real psync_task_free() rather than a local replica
- Intercept pthread_mutex_lock/unlock and pmem_free via --wrap linker
  flags to observe lock discipline and detect destroy invocations

Update the Makefile test_ptask_free target to link pclsync/ptask_free.c
and pass the required --wrap flags. Production build unchanged: ptask_free.o
is picked up automatically by the existing wildcard COBJ rule.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Levi Neely 2026-03-10 19:32:05 +01:00
parent 3b41bb3998
commit 67c99c5e06
5 changed files with 303 additions and 265 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
Makefile
View File

@ -191,8 +191,12 @@ tests/test_ptools_params: $(UNIT_DIR)/test_ptools_params.c $(LIBDIR)/ptools.c $(
tests/test_pfs_lock_ordering: $(UNIT_DIR)/test_pfs_lock_ordering.c
$(CC) $(TEST_CFLAGS) $(CFLAGS) -o $@ $< -lpthread
tests/test_ptask_free: $(UNIT_DIR)/test_ptask_free.c
$(CC) $(TEST_CFLAGS) $(CFLAGS) -o $@ $< -lpthread
tests/test_ptask_free: $(UNIT_DIR)/test_ptask_free.c $(LIBDIR)/ptask_free.c
$(CC) $(TEST_CFLAGS) $(CFLAGS) -o $@ $^ \
-Wl,--wrap=pthread_mutex_lock \
-Wl,--wrap=pthread_mutex_unlock \
-Wl,--wrap=pmem_free \
-lpthread
tests/test_prun: $(UNIT_DIR)/test_prun.c $(LIBDIR)/prun.c $(LIBDIR)/pdbg.c $(LIBDIR)/pmem.c $(LIBDIR)/putil.c $(LIBDIR)/ppath.c tests/stubs/test_stubs.c
$(CC) -D_POSIX_C_SOURCE=199309L $(CFLAGS) -o $@ $^ \

77
pclsync/ptask.c
View File

@ -48,20 +48,12 @@
#include "pstatus.h"
#include "psys.h"
#include "ptask.h"
#include "ptask_free_internal.h"
#include "ptree.h"
#include "pupload.h"
#define get_len(t) (sizeof(t) - offsetof(t, request))
#define PSYNC_TASK_STATUS_RUNNING 0
#define PSYNC_TASK_STATUS_READY 1
#define PSYNC_TASK_STATUS_DONE 2
#define PSYNC_TASK_STATUS_RETURNED 3
// #define PSYNC_WAIT_ANYBODY -1 // unused, but may be important later
#define PSYNC_WAIT_NOBODY -2
#define PSYNC_WAIT_FREED -3
#define TASK_TYPE_EXIT 0
#define TASK_TYPE_FILE_DWL 1
#define TASK_TYPE_FILE_DWL_NM 2
@ -163,22 +155,6 @@ typedef struct {
int fd;
} download_context_t;
struct psync_task_t_ {
psync_task_callback_t callback;
void *param;
pthread_cond_t cond;
int id;
int status;
};
struct psync_task_manager_t_ {
pthread_mutex_t mutex;
int taskcnt;
int refcnt;
int waitfor;
struct psync_task_t_ tasks[];
};
static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
static int running = 0;
static int sockd = INVALID_SOCKET;
@ -845,35 +821,6 @@ static int task_send_async(const void *task, size_t len) {
return ret;
}
static void psync_task_destroy(psync_task_manager_t tm) {
int i;
for (i = 0; i < tm->taskcnt; i++)
pthread_cond_destroy(&tm->tasks[i].cond);
pthread_mutex_destroy(&tm->mutex);
pmem_free(PMEM_SUBSYS_OTHER, tm);
}
static void psync_task_dec_refcnt(psync_task_manager_t tm) {
int refcnt;
pthread_mutex_lock(&tm->mutex);
refcnt = --tm->refcnt;
pthread_mutex_unlock(&tm->mutex);
if (!refcnt)
psync_task_destroy(tm);
}
static psync_task_manager_t psync_get_manager_of_task(struct psync_task_t_ *t) {
return (psync_task_manager_t)(((char *)(t - t->id)) -
offsetof(struct psync_task_manager_t_, tasks));
}
static void psync_task_entry(void *ptr) {
struct psync_task_t_ *t;
t = (struct psync_task_t_ *)ptr;
t->callback(ptr, t->param);
psync_task_dec_refcnt(psync_get_manager_of_task(t));
}
void ptask_ldir_mk(psync_syncid_t syncid,
psync_folderid_t folderid,
psync_folderid_t localfolderid) {
@ -1181,28 +1128,6 @@ void *psync_task_papi_result(psync_task_manager_t tm, int id) {
return ret;
}
void psync_task_free(psync_task_manager_t tm) {
int refcnt, i;
pthread_mutex_lock(&tm->mutex);
if (tm->refcnt == 1) {
pthread_mutex_unlock(&tm->mutex);
psync_task_destroy(tm);
}
else {
tm->waitfor = PSYNC_WAIT_FREED;
for (i = 0; i < tm->taskcnt; i++)
if (tm->tasks[i].status == PSYNC_TASK_STATUS_READY) {
tm->tasks[i].status = PSYNC_TASK_STATUS_RETURNED;
pthread_cond_signal(&tm->tasks[i].cond);
}
refcnt = --tm->refcnt;
pthread_mutex_unlock(&tm->mutex);
if (!refcnt) {
psync_task_destroy(tm);
}
}
}
int psync_task_complete(void *h, void *data) {
psync_task_manager_t tm;
struct psync_task_t_ *t;

72
pclsync/ptask_free.c Normal file
View File

@ -0,0 +1,72 @@
/*
* ptask_free.c — psync_task_manager_t lifecycle: destroy and free.
*
* Extracted from ptask.c as a separately compilable unit so that
* tests/unit-tests/test_ptask_free.c can link against this file alone,
* without dragging in ptask.c's heavyweight dependencies (papi, psql,
* pdeflate, …).
*
* Fix: b92a389 — mutex is held during the refcnt check in all paths.
*/
#include <pthread.h>
#include <stddef.h>
#include "pmem.h"
#include "ptask_free_internal.h"
/* ------------------------------------------------------------------ */
/* Static helpers */
/* ------------------------------------------------------------------ */
static void psync_task_destroy(psync_task_manager_t tm) {
int i;
for (i = 0; i < tm->taskcnt; i++)
pthread_cond_destroy(&tm->tasks[i].cond);
pthread_mutex_destroy(&tm->mutex);
pmem_free(PMEM_SUBSYS_OTHER, tm);
}
static void psync_task_dec_refcnt(psync_task_manager_t tm) {
int refcnt;
pthread_mutex_lock(&tm->mutex);
refcnt = --tm->refcnt;
pthread_mutex_unlock(&tm->mutex);
if (!refcnt)
psync_task_destroy(tm);
}
/* ------------------------------------------------------------------ */
/* Thread entry-point (non-static: referenced from psync_task_run_tasks
* in ptask.c via the forward declaration in ptask_free_internal.h) */
/* ------------------------------------------------------------------ */
void psync_task_entry(void *ptr) {
struct psync_task_t_ *t = (struct psync_task_t_ *)ptr;
t->callback(ptr, t->param);
psync_task_dec_refcnt(psync_get_manager_of_task(t));
}
/* ------------------------------------------------------------------ */
/* Public API */
/* ------------------------------------------------------------------ */
void psync_task_free(psync_task_manager_t tm) {
int refcnt, i;
pthread_mutex_lock(&tm->mutex);
if (tm->refcnt == 1) {
pthread_mutex_unlock(&tm->mutex);
psync_task_destroy(tm);
} else {
tm->waitfor = PSYNC_WAIT_FREED;
for (i = 0; i < tm->taskcnt; i++)
if (tm->tasks[i].status == PSYNC_TASK_STATUS_READY) {
tm->tasks[i].status = PSYNC_TASK_STATUS_RETURNED;
pthread_cond_signal(&tm->tasks[i].cond);
}
refcnt = --tm->refcnt;
pthread_mutex_unlock(&tm->mutex);
if (!refcnt)
psync_task_destroy(tm);
}
}

75
pclsync/ptask_free_internal.h Normal file
View File

@ -0,0 +1,75 @@
/*
* ptask_free_internal.h — internal layout of psync_task_manager_t.
*
* Included by:
* - pclsync/ptask_free.c (production lifecycle code)
* - pclsync/ptask.c (rest of the task subsystem)
* - tests/unit-tests/test_ptask_free.c (struct-level access in tests)
*
* NOT part of the public API. Do not include from general application code.
*/
#ifndef PTASK_FREE_INTERNAL_H
#define PTASK_FREE_INTERNAL_H
#include <pthread.h>
#include <stddef.h>
/*
* Provide minimal forward declarations when this header is included
* standalone (e.g. from the test). When ptask.h has already been
* included its include guard (_PSYNC_TASKS_H) suppresses the duplicates.
*/
#ifndef _PSYNC_TASKS_H
typedef void (*psync_task_callback_t)(void *, void *);
struct psync_task_manager_t_;
typedef struct psync_task_manager_t_ *psync_task_manager_t;
#endif
/* Task status values */
#define PSYNC_TASK_STATUS_RUNNING 0
#define PSYNC_TASK_STATUS_READY 1
#define PSYNC_TASK_STATUS_DONE 2
#define PSYNC_TASK_STATUS_RETURNED 3
/* waitfor sentinel values */
// #define PSYNC_WAIT_ANYBODY -1 /* unused, but may be useful later */
#define PSYNC_WAIT_NOBODY -2
#define PSYNC_WAIT_FREED -3
struct psync_task_t_ {
psync_task_callback_t callback;
void *param;
pthread_cond_t cond;
int id;
int status;
};
struct psync_task_manager_t_ {
pthread_mutex_t mutex;
int taskcnt;
int refcnt;
int waitfor;
struct psync_task_t_ tasks[];
};
/*
* Helper: given a pointer to an individual task, return the owning manager.
* Declared static inline so both ptask.c and ptask_free.c can use it
* without any linkage conflict.
*/
static inline psync_task_manager_t
psync_get_manager_of_task(struct psync_task_t_ *t) {
return (psync_task_manager_t)(((char *)(t - t->id)) -
offsetof(struct psync_task_manager_t_, tasks));
}
/* Defined in ptask_free.c; declared in ptask.h for normal callers. */
void psync_task_free(psync_task_manager_t tm);
/*
* psync_task_entry is defined in ptask_free.c and used as a thread
* entry-point in psync_task_run_tasks (ptask.c).
*/
void psync_task_entry(void *ptr);
#endif /* PTASK_FREE_INTERNAL_H */

182
tests/unit-tests/test_ptask_free.c
View File

@ -6,118 +6,74 @@
* 2. refcnt>1, last ref: lock acquired, refcnt decremented, destroy called
* 3. refcnt>1, not last ref: refcnt decremented, destroy NOT called
* 4. READY tasks get signaled (status→RETURNED) when freed with refcnt>1
* 5. mutex is acquired before the refcnt check in all paths
*
* The mutex is held during the refcnt check in all paths — the core fix.
* We verify this by intercepting pthread_mutex_lock/unlock with counters
* and confirming lock is held before destroy is invoked.
* Links against the real pclsync/ptask_free.c (production code).
* Uses --wrap linker flags to intercept pthread_mutex_lock/unlock and
* pmem_free so we can observe lock discipline and detect destroy calls
* without reimplementing any production logic inline.
*/
#define _POSIX_C_SOURCE 200809L
#include <pthread.h>
#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
/* Internal struct layout — exposes psync_task_manager_t_ for make_tm() */
#include "ptask_free_internal.h"
#include "pmem.h"
/* ------------------------------------------------------------------ */
/* Intercept controls */
/* __wrap / __real declarations */
/* ------------------------------------------------------------------ */
int __real_pthread_mutex_lock(pthread_mutex_t *m);
int __real_pthread_mutex_unlock(pthread_mutex_t *m);
void __real_pmem_free(pmem_subsystem_t subsystem, void *ptr);
/* ------------------------------------------------------------------ */
/* Intercept state */
/* ------------------------------------------------------------------ */
static int g_lock_calls = 0;
static int g_unlock_calls = 0;
static int g_destroy_calls = 0;
static int g_free_calls = 0;
static int g_lock_held_at_destroy = 0; /* was lock held when destroy fired? */
/* ------------------------------------------------------------------ */
/* Inline struct replica (mirrors ptask.c exactly) */
/* ------------------------------------------------------------------ */
#define PSYNC_TASK_STATUS_RUNNING 0
#define PSYNC_TASK_STATUS_READY 1
#define PSYNC_TASK_STATUS_DONE 2
#define PSYNC_TASK_STATUS_RETURNED 3
#define PSYNC_WAIT_NOBODY -2
#define PSYNC_WAIT_FREED -3
typedef void (*psync_task_callback_t)(void *, void *);
struct psync_task_t_ {
psync_task_callback_t callback;
void *param;
pthread_cond_t cond;
int id;
int status;
};
struct psync_task_manager_t_ {
pthread_mutex_t mutex;
int taskcnt;
int refcnt;
int waitfor;
struct psync_task_t_ tasks[];
};
typedef struct psync_task_manager_t_ *psync_task_manager_t;
/* ------------------------------------------------------------------ */
/* Mock implementations */
/* ------------------------------------------------------------------ */
/* Track lock depth so we know if lock is held when destroy fires */
static int g_destroy_calls = 0; /* incremented by __wrap_pmem_free */
static int g_lock_depth = 0;
static int g_lock_held_at_destroy = 0; /* 1 = mutex was unlocked when destroy fired */
static int mock_mutex_lock(pthread_mutex_t *m) {
/* ------------------------------------------------------------------ */
/* Wrap implementations */
/* ------------------------------------------------------------------ */
int __wrap_pthread_mutex_lock(pthread_mutex_t *m) {
g_lock_calls++;
g_lock_depth++;
return pthread_mutex_lock(m);
return __real_pthread_mutex_lock(m);
}
static int mock_mutex_unlock(pthread_mutex_t *m) {
int __wrap_pthread_mutex_unlock(pthread_mutex_t *m) {
g_unlock_calls++;
g_lock_depth--;
return pthread_mutex_unlock(m);
return __real_pthread_mutex_unlock(m);
}
static void mock_pmem_free(void *p) {
g_free_calls++;
free(p);
}
static void mock_psync_task_destroy(psync_task_manager_t tm) {
/*
* psync_task_destroy() calls pmem_free() as its last act.
* We intercept it to count destroy invocations and capture lock state.
* We call free() directly because make_tm() allocates with malloc().
*/
void __wrap_pmem_free(pmem_subsystem_t subsystem, void *ptr) {
(void)subsystem;
g_destroy_calls++;
g_lock_held_at_destroy = (g_lock_depth == 0); /* should be 0: unlocked before destroy */
int i;
for (i = 0; i < tm->taskcnt; i++)
pthread_cond_destroy(&tm->tasks[i].cond);
pthread_mutex_destroy(&tm->mutex);
mock_pmem_free(tm);
}
/* ------------------------------------------------------------------ */
/* Replica of psync_task_free from the fixed branch */
/* ------------------------------------------------------------------ */
static void test_psync_task_free(psync_task_manager_t tm) {
int refcnt, i;
mock_mutex_lock(&tm->mutex);
if (tm->refcnt == 1) {
mock_mutex_unlock(&tm->mutex);
mock_psync_task_destroy(tm);
} else {
tm->waitfor = PSYNC_WAIT_FREED;
for (i = 0; i < tm->taskcnt; i++)
if (tm->tasks[i].status == PSYNC_TASK_STATUS_READY) {
tm->tasks[i].status = PSYNC_TASK_STATUS_RETURNED;
pthread_cond_signal(&tm->tasks[i].cond);
}
refcnt = --tm->refcnt;
mock_mutex_unlock(&tm->mutex);
if (!refcnt)
mock_psync_task_destroy(tm);
}
g_lock_held_at_destroy = (g_lock_depth == 0); /* should be 0 = unlocked */
free(ptr);
}
/* ------------------------------------------------------------------ */
/* Helpers */
/* ------------------------------------------------------------------ */
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)
@ -126,16 +82,15 @@ static void reset(void) {
g_lock_calls = 0;
g_unlock_calls = 0;
g_destroy_calls = 0;
g_free_calls = 0;
g_lock_depth = 0;
g_lock_held_at_destroy = 0;
}
/* Allocate and initialize a task manager with `cnt` tasks */
/* Allocate and initialise a task manager with `cnt` tasks, refcnt=`refcnt` */
static psync_task_manager_t make_tm(int cnt, int refcnt) {
size_t sz = sizeof(struct psync_task_manager_t_) +
cnt * sizeof(struct psync_task_t_);
psync_task_manager_t tm = malloc(sz);
psync_task_manager_t tm = (psync_task_manager_t)malloc(sz);
memset(tm, 0, sz);
pthread_mutex_init(&tm->mutex, NULL);
tm->taskcnt = cnt;
@ -151,40 +106,44 @@ static psync_task_manager_t make_tm(int cnt, int refcnt) {
}
/* ------------------------------------------------------------------ */
/* Tests */
/* Tests — call the real psync_task_free() */
/* ------------------------------------------------------------------ */
/* refcnt=1: destroy called, mutex unlocked before destroy */
/* refcnt=1: destroy called once, mutex unlocked before destroy */
static void test_single_owner_free(void) {
reset();
psync_task_manager_t tm = make_tm(2, 1);
test_psync_task_free(tm); /* tm is freed inside */
psync_task_free(tm); /* tm freed inside via __wrap_pmem_free */
if (g_destroy_calls != 1)
FAIL("single owner: destroy called once", "destroy_calls=%d", g_destroy_calls);
FAIL("single owner: destroy called once",
"destroy_calls=%d", g_destroy_calls);
else if (!g_lock_held_at_destroy)
FAIL("single owner: mutex unlocked before destroy", "lock_depth was non-zero at destroy");
FAIL("single owner: mutex unlocked before destroy",
"lock_depth was non-zero at destroy");
else if (g_lock_calls != 1 || g_unlock_calls != 1)
FAIL("single owner: lock/unlock balanced", "lock=%d unlock=%d", g_lock_calls, g_unlock_calls);
FAIL("single owner: lock/unlock balanced",
"lock=%d unlock=%d", g_lock_calls, g_unlock_calls);
else
PASS("single owner free: destroy called once, mutex unlocked before destroy");
}
/* refcnt=2, free last ref manually: destroy called after second decrement */
/* refcnt already at 1 when we call free: same result as single-owner */
static void test_last_ref_destroys(void) {
reset();
psync_task_manager_t tm = make_tm(1, 2);
/* Simulate first ref already released: lower refcnt to 1 without locking */
/* Simulate the other ref already gone */
tm->refcnt = 1;
test_psync_task_free(tm); /* this is now the last ref */
psync_task_free(tm);
if (g_destroy_calls == 1 && g_lock_held_at_destroy)
PASS("last ref free (refcnt path 1): destroy called, mutex unlocked before destroy");
PASS("last ref free: destroy called, mutex unlocked before destroy");
else
FAIL("last ref free", "destroy_calls=%d lock_held_at_destroy=%d",
FAIL("last ref free",
"destroy_calls=%d lock_held_at_destroy=%d",
g_destroy_calls, g_lock_held_at_destroy);
}
@ -193,22 +152,24 @@ static void test_not_last_ref_no_destroy(void) {
reset();
psync_task_manager_t tm = make_tm(1, 2);
test_psync_task_free(tm);
psync_task_free(tm);
if (g_destroy_calls != 0)
FAIL("not last ref: no destroy", "destroy_calls=%d", g_destroy_calls);
FAIL("not last ref: no destroy",
"destroy_calls=%d", g_destroy_calls);
else if (tm->refcnt != 1)
FAIL("not last ref: refcnt decremented to 1", "refcnt=%d", tm->refcnt);
FAIL("not last ref: refcnt decremented to 1",
"refcnt=%d", tm->refcnt);
else
PASS("not last ref: no destroy, refcnt decremented to 1");
/* Manual cleanup since we didn't destroy */
/* Manual cleanup since psync_task_destroy was not called */
pthread_cond_destroy(&tm->tasks[0].cond);
pthread_mutex_destroy(&tm->mutex);
free(tm);
}
/* READY tasks get RETURNED status when freed with refcnt>1 */
/* READY tasks get RETURNED status + cond signalled when freed with refcnt>1 */
static void test_ready_tasks_signaled(void) {
reset();
psync_task_manager_t tm = make_tm(3, 2);
@ -216,7 +177,7 @@ static void test_ready_tasks_signaled(void) {
tm->tasks[1].status = PSYNC_TASK_STATUS_READY;
tm->tasks[2].status = PSYNC_TASK_STATUS_DONE;
test_psync_task_free(tm);
psync_task_free(tm);
int ok = (tm->tasks[0].status == PSYNC_TASK_STATUS_RUNNING &&
tm->tasks[1].status == PSYNC_TASK_STATUS_RETURNED &&
@ -231,27 +192,28 @@ static void test_ready_tasks_signaled(void) {
tm->tasks[0].status, tm->tasks[1].status,
tm->tasks[2].status, tm->waitfor);
/* Cleanup */
/* Manual cleanup */
int i;
for (i = 0; i < 3; i++) pthread_cond_destroy(&tm->tasks[i].cond);
pthread_mutex_destroy(&tm->mutex);
free(tm);
}
/* Lock is acquired before refcnt is read (core fix) */
/* Mutex acquired before refcnt is inspected (core fix) */
static void test_lock_before_refcnt_check(void) {
reset();
psync_task_manager_t tm = make_tm(1, 1);
/* We can only verify indirectly: lock_calls >= 1 before destroy fires.
* g_lock_held_at_destroy==1 means lock was acquired and released before destroy. */
test_psync_task_free(tm);
psync_task_free(tm);
/* g_lock_calls >= 1 means lock was acquired; g_lock_held_at_destroy = 1
* means it was released cleanly before destroy fired */
if (g_lock_calls >= 1 && g_lock_held_at_destroy)
PASS("mutex acquired before refcnt check; unlocked cleanly before destroy");
else
FAIL("lock before refcnt check",
"lock_calls=%d lock_held_at_destroy=%d", g_lock_calls, g_lock_held_at_destroy);
"lock_calls=%d lock_held_at_destroy=%d",
g_lock_calls, g_lock_held_at_destroy);
}
/* ------------------------------------------------------------------ */