!pthread: implement cancellation points - #524
adamgreloch wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements POSIX thread cancellation support in libphoenix, introducing cancellation points, state management, and wrapping various blocking system calls. While the addition of cancellation handling is a significant step, the current implementation has several critical issues. Most notably, executing cleanup handlers and thread-specific data destructors on the canceller thread instead of the victim thread violates POSIX and risks severe context corruption. Furthermore, failed signal posting can leave victim threads in a corrupted state, cancelled detached threads permanently leak resources, and several system call wrappers (such as open and sendmsg) risk leaking heap-allocated memory if cancelled before freeing. Addressing these concurrency, resource management, and POSIX compliance issues is essential.
| _pthread_do_cleanup(ctx); | ||
| ctx->retval = (void *)PTHREAD_CANCELED; | ||
| id = ctx->id; | ||
| mutexUnlock(pthread_common.pthread_list_lock); | ||
|
|
||
| pthread_key_cleanup(ctx); |
There was a problem hiding this comment.
In pthread_cancel, the cleanup handlers (_pthread_do_cleanup(ctx)) and thread-specific data destructors (pthread_key_cleanup(ctx)) are executed directly by the canceller thread instead of the victim thread. This violates POSIX requirements and introduces severe concurrency and correctness issues. Specifically, any cleanup handler or destructor that accesses thread-local storage (TLS via __thread or pthread_getspecific), or calls pthread_self(), will operate on the canceller thread's context rather than the victim's. This can lead to silent data corruption, resource leaks, or crashes.
| err = signalPost(getpid(), id, signal_cancel); | ||
| if (err != 0) { | ||
| /* Nothing was destroyed, so withdraw the destroy claim. */ | ||
| (void)_pthread_cancel_clear(ctx, CANCEL_INPROGRESS_BIT); | ||
| } |
There was a problem hiding this comment.
If signalPost(getpid(), id, signal_cancel) fails, the canceller thread withdraws the destroy claim by clearing CANCEL_INPROGRESS_BIT. The victim thread (which was waiting in _pthread_wait_for_cancel) will then resume execution. However, the canceller thread has already executed _pthread_do_cleanup(ctx) and pthread_key_cleanup(ctx). This leaves the victim thread running in a corrupted state where its cleanup handlers and thread-specific data have been prematurely destroyed/cleared, leading to undefined behavior.
|
|
||
| if (err == EOK) { | ||
| err = -condWait(cond->condh, mutex->mutexh, 0); | ||
| err = -CANCELLATION_POINT(int, condWait, (cond->condh, mutex->mutexh, 0)); |
There was a problem hiding this comment.
When a thread blocked in pthread_cond_wait is cancelled, POSIX requires that the thread re-acquire the associated mutex before calling any cancellation cleanup handlers. However, because the victim thread is terminated directly by signal_cancel and the cleanup handlers are executed by the canceller thread, the mutex is never re-acquired. If a cleanup handler expects the mutex to be locked (which is standard practice to allow safe unlocking/cleanup), calling pthread_mutex_unlock will fail or cause undefined behavior.
| _pthread_do_cleanup(ctx); | ||
| ctx->retval = (void *)PTHREAD_CANCELED; | ||
| id = ctx->id; | ||
| mutexUnlock(pthread_common.pthread_list_lock); | ||
|
|
||
| pthread_key_cleanup(ctx); | ||
|
|
||
| err = signalPost(getpid(), id, signal_cancel); | ||
| if (err != 0) { | ||
| /* Nothing was destroyed, so withdraw the destroy claim. */ | ||
| (void)_pthread_cancel_clear(ctx, CANCEL_INPROGRESS_BIT); | ||
| } | ||
|
|
||
| pthread_ctx_put(ctx); |
There was a problem hiding this comment.
When a detached thread is cancelled, it is terminated by the kernel via signal_cancel. Because it is terminated abruptly, it never reaches pthread_do_exit where _pthread_release(ctx, 1) would normally be called to unmap its stack and free its pthread_ctx. Since detached threads cannot be joined, their resources (stack and context) are permanently leaked. A mechanism (such as a deferred reclamation queue or a reaper) is needed to safely reclaim resources of cancelled detached threads.
| do { | ||
| err = CANCELLATION_POINT(int, sys_open, (canonical, oflag, mode)); | ||
| } while (err == -EINTR); | ||
|
|
||
| free(canonical); |
There was a problem hiding this comment.
Several wrapper functions (such as open, select, sendmsg, and recvmsg) allocate heap memory (e.g., canonical, pfd, buf) immediately before calling a cancellation point, and free it afterwards. If the cancellation point is triggered, the thread terminates without executing the subsequent free() calls, resulting in memory leaks. To prevent this, these functions should use pthread_cleanup_push and pthread_cleanup_pop to ensure that allocated resources are freed even if the thread is cancelled.
pthread_cleanup_push(free, canonical);
do {
err = CANCELLATION_POINT(int, sys_open, (canonical, oflag, mode));
} while (err == -EINTR);
pthread_cleanup_pop(1);| len = CANCELLATION_POINT(ssize_t, sys_sendmsg, (socket, &_msg, flags)); | ||
| free(buf); |
There was a problem hiding this comment.
If the cancellation point is triggered during sys_sendmsg, the temporary buffer buf allocated for scatter-gather copying will be leaked. Use pthread_cleanup_push and pthread_cleanup_pop to ensure the buffer is freed upon cancellation.
pthread_cleanup_push(free, buf);
len = CANCELLATION_POINT(ssize_t, sys_sendmsg, (socket, &_msg, flags));
pthread_cleanup_pop(1);
011c80a to
7516a25
Compare
Assisted-by: claude-opus-5 Fixes: phoenix-rtos/phoenix-rtos-project#1645 TASK: RTOS-1356
7516a25 to
aea8c0a
Compare
1 participant
Assisted-by: claude-opus-5
Fixes: phoenix-rtos/phoenix-rtos-project#1645
TASK: RTOS-1356
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment