Bug#459322: libc6-dev: implementation of pthread_cleanup_pop_restore_np
One last attempt. I've copied the manpage exactly into the code. The
manpage says two example codes are "functionally equivalent" when
explaining what "pthread_cleanup_push_defer_np and
pthread_cleanup_pop_restore_np" do. If one uses one of the manpage
example codes, everything works in the testcase code I supplied. If one
uses the other of the manpage example example codes in the testcase code
I supplied, then the testcase code I supplied fails.
Summary:
-------
The manpage (pthread_cleanup_pop_restore_np(3)
example code that fails is:
pthread_cleanup_push_defer_np(routine, arg);
pthread_cleanup_pop_defer_np(execute);
The manpage example code that works is:
{ int oldtype;
pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, &oldtype);
pthread_cleanup_push(routine, arg);
...
pthread_cleanup_pop(execute);
pthread_setcanceltype(oldtype, NULL);
}
Reasoning:
----------
This is what the manpage says about them:
=========================
The following sequence
pthread_cleanup_push_defer_np(routine, arg);
pthread_cleanup_pop_defer_np(execute);
is functionally equivalent to (but more compact and more efficient
than)
{ int oldtype;
pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, &oldtype);
pthread_cleanup_push(routine, arg);
...
pthread_cleanup_pop(execute);
pthread_setcanceltype(oldtype, NULL);
}
=========================
Wel, the manpage is wrong. You choose where the bug is. But it's a bug.
Proof of claim:
--------------
The appended testcase code when compiled with
gcc -Wall -g -o testp-2 testp-2.c
will use the following sequence from the manpage
in its rather trivial thread function code, and fail as a result:
=========================
pthread_cleanup_push_defer_np (routine, arg);
// now back in deferred cancellation mode
err = pthread_mutex_lock (&mutex);
if (err)
print_error (err, "pthread_mutex_lock", __LINE__);
mcount++;
execute = !err;
// here we go back to async mode and THEN unlock, too late
pthread_cleanup_pop_restore_np (execute);
=========================
This exactly matches the manual page. You can see the two functions
that the manpage names:
pthread_cleanup_push_defer_np(routine, arg);
pthread_cleanup_pop_defer_np(execute);
at the beginning and end of the code sequence above (hey, the man page
is missing an ellipsis between the two lines!).
This is what happens when one runs the compliled code:
% ./testp-2
(3908) parent created thread OK
(3908) parent detached thread OK
(3908) parent sleeps 2s
(3909) child set async cancellation mode
(3909) child sleeps 1s
(3909) child wakes and starts cycling lock/unlock of mutex
(3908) parent wakes after sleep
(3908) parent starts sending cancels to child
(3908) parent spots child died
!!child died leaving lock set!!
1 threads created
1 cancel signals sent
2093121 lock cycles completed
The termination printout says that the thread function that was started
left a mutex locked after dying through receiving a cancel message. That
means it did not run the cleanup routine as it was supposed to.
On the other hand, if one compiles the test code with
gcc -Wall -DMY_DONT_USE_DEFER_NP -g -pthread testp-2.c -o testp-2
Then the code will use the alternative sequence from the manpage in
the interior of its thread function (and attention, that sequence
has another defect ... but one that is not exercised here; it
potentially can run a cleanup routine twice or more, but that depends on
implementation details within the pthread library). This code sequence is:
=========================
int oldtype;
pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, &oldtype);
pthread_cleanup_push(routine, arg);
// now back in deferred cancellation mode
err = pthread_mutex_lock (&mutex);
if (err)
print_error (err, "pthread_mutex_lock", __LINE__);
mcount++;
execute = !err;
pthread_cleanup_pop(execute);
pthread_setcanceltype(oldtype, NULL);
=========================
and once again you can see that it is exactly the manpage code
{ int oldtype;
pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, &oldtype);
pthread_cleanup_push(routine, arg);
...
pthread_cleanup_pop(execute);
pthread_setcanceltype(oldtype, NULL);
}
with the ellipsis replaced by those central 6 lines of code of mine
(which take a lock, print an error message if an error occurred, and
increment a count and then release the lock if taking the lock was
successful).
This time the compiled code runs without error, forever:
% ./testp-2
(3987) parent created thread OK
(3987) parent detached thread OK
(3987) parent sleeps 2s
(3988) child set async cancellation mode
(3988) child sleeps 1s
(3988) child wakes and starts cycling lock/unlock of mutex
(3987) parent wakes after sleep
(3987) parent starts sending cancels to child
(3987) parent spots child died
(3987) parent sees child cleaned up OK, restarting
(3987) parent created thread OK
(3987) parent detached thread OK
(3987) parent sleeps 2s
(3989) child set async cancellation mode
(3989) child sleeps 1s
(3989) child wakes and starts cycling lock/unlock of mutex
(3987) parent wakes after sleep
(3987) parent starts sending cancels to child
(3987) parent spots child died
(3987) parent sees child cleaned up OK, restarting
(3987) parent created thread OK
(3987) parent detached thread OK
(3987) parent sleeps 2s
(3990) child set async cancellation mode
(3990) child sleeps 1s
(3990) child wakes and starts cycling lock/unlock of mutex
(3987) parent wakes after sleep
(3987) parent starts sending cancels to child
(3987) parent spots child died
(3987) parent sees child cleaned up OK, restarting
(3987) parent created thread OK
(3987) parent detached thread OK
(3987) parent sleeps 2s
(3991) child set async cancellation mode
(3991) child sleeps 1s
(3991) child wakes and starts cycling lock/unlock of mutex
(3987) parent wakes after sleep
(3987) parent starts sending cancels to child
(3987) parent spots child died
(3987) parent sees child cleaned up OK, restarting
(3987) parent created thread OK
(3987) parent detached thread OK
(3987) parent sleeps 2s
(3992) child set async cancellation mode
(3992) child sleeps 1s
(3992) child wakes and starts cycling lock/unlock of mutex
(3987) parent wakes after sleep
(3987) parent starts sending cancels to child
(3987) parent spots child died
(3987) parent sees child cleaned up OK, restarting
(3987) parent created thread OK
(3987) parent detached thread OK
(3987) parent sleeps 2s
(3993) child set async cancellation mode
(3993) child sleeps 1s
...
and that's what SHOULD happen.
Conclusion:
----------
The manpage is wrong or the push_def_np/pop_defer_np
implementation is wrong.
Judgement:
--------
In my opinion it is clearly the latter that is the case.
Proposed fix:
-------------
Here's the fix, for pthread.h, at least for my environment. There
are three areas to fix in pthread.h, one for c++, one for c with
__EXCEPTIONS set, and one for c without __EXCEPTIONS set. I only fixed
the one that matches my environment (I think that's C, with, as I
recall).
--- pthread.h.orig 2007-12-19 21:48:54.000000000 +0100
+++ pthread.h 2008-01-18 16:09:58.000000000 +0100
@@ -667,11 +669,12 @@
/* Install a cleanup handler as pthread_cleanup_push does, but also
saves the current cancellation type and sets it to deferred
cancellation. */
# define pthread_cleanup_push_defer_np(routine, arg) \
do { \
__pthread_unwind_buf_t __cancel_buf; \
void (*__cancel_routine) (void *) = (routine); \
void *__cancel_arg = (arg); \
+ int __ran_cancel_routine = 0; \
int not_first_call = __sigsetjmp ((struct __jmp_buf_tag *) \
__cancel_buf.__cancel_jmp_buf, 0); \
if (__builtin_expect (not_first_call, 0)) \
@@ -691,9 +695,13 @@
pthread_cleanup_push_defer was called. */
# define pthread_cleanup_pop_restore_np(execute) \
} while (0); \
+ if (execute) { \
+ if (!__ran_cancel_routine++) \
+ __cancel_routine (__cancel_arg); \
+ else \
+ __ran_cancel_routine--; \
+ } \
__pthread_unregister_cancel_restore (&__cancel_buf); \
- if (execute) \
- __cancel_routine (__cancel_arg); \
} while (0)
extern void __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute;
Explanation of fix:
-------------------
The fix inverts the order of running in pthread.h
__pthread_unregister_cancel_restore
__cancel_routine
in pthread_cleanup_pop_restore_np so that it matches the man page
explanation of what pthread_cleanup_pop_restore_np is "functionally
equivalent to". To prevent multiple possible execs of the cleanup
routine as a result (a potential defect with the code that the manpage
proposes), it also introduces a counter for how many times it has been
run, and limits it to once thereby.
I'll repeat an explanation of the testcase code, and then append it:
Testcase code summary:
----------------
The testcase code simply does
1) launch a child thread
the child switches to async cancellation mode
the child sleeps 1s
the child starts cycling forever taking a mutex and releasing it
again as fast as it can, using a pthread_cleanup_push_defer_np
and pthread_cleanup_pop_restore_np pair to protect the
lock/unlock.
2) sleep 2s second
3) start sending cancel messages to the child in a loop every 1ms.
if the cancel message fails with ESRCH, meåning the child has
died, examine the state of the mutex with trylock to see if
the child left it locked or unlocked.
locked means the child failed to run the cleanup function,
and the testcase is successful and showing it up.
unlocked means the child got lucky before it died, and we go
back to (1)
What the testcase should do:
---------------------------
If the testcase code exposes the bug in the implementation of the
pthread macros, it will terminate with a message like:
!!child died leaving lock set!!
1 threads created
1 cancel signals sent
1670442 lock cycles completed
summarising the statsistics.
If the testcase exposes no bug, it will continue cycling "forever",
until interrupted, printing progress messages every second or so to screen.
Here is the code:
/* compile with gcc -Wall -g testp-2.c -o testp-2
* or gcc -Wall -DMY_DONT_USE_DEFER_NP -g testp-2.c -o testp-2
*
* (the first shows the error condition, the second avoids it by using
* an equivalent shown on the manpage for pthread_cleanup_pop_restore_np).
*
* run as
*
* ./testp-2
*
*/
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <error.h>
#include <errno.h>
#include <time.h>
#include <signal.h>
#include <sys/types.h>
#define _GNU_SOURCE /* or _BSD_SOURCE or _SVID_SOURCE */
#include <unistd.h>
#include <sys/syscall.h>
#define __USE_GNU 1
#include <pthread.h>
extern void __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute;
static pthread_mutex_t mutex;
static int kcount;
static int tcount;
static int mcount;
static int debug = 1;
/*
* Merely a printout function, in case of errors (other than the
* expected one).
*/
static void
print_error (int err, char *fnname, int line)
{
error_at_line (0, err, __FILE__, line, "err %d in function %s", err, fnname);
}
/*
* Make noise on stderr from time to time if debug is set
*/
#define DEBUG(s ...) if (debug) { \
fprintf(stderr, "(%ld) ", syscall(SYS_gettid)); \
fprintf(stderr, s); \
}
/*
* A trivial thread function.
*
* 1 the child switches to async cancellation mode
* 2 the child sleeps 1s
* 3 the child starts cycling forever
* taking a mutex and releasing it again as fast as it can
*
* by default it uses a pthread_cleanup_push_defer_np
* and pthread_cleanup_pop_restore_np pair to protect the
* lock/unlock.
* if compiled with MY_DONT_USE_DEFER_NP, it uses an alternative code
* sequence stated to be equivalent on the manpage.
*
*/
static void *
tfn (void *tdata)
{
int err = pthread_setcanceltype (PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
if (err) {
print_error (err, "pthread_setcanceltype", __LINE__);
return NULL;
}
DEBUG ("child set async cancellation mode\n");
/* now with cancellation enabled and async */
DEBUG ("child sleeps 1s\n");
sleep (1);
DEBUG ("child wakes and starts cycling lock/unlock of mutex\n");
while (1) {
void *arg = &mutex;
void (*routine)(void *) = (void(*)(void *)) pthread_mutex_unlock;
int execute;
#if !defined(MY_DONT_USE_DEFER_NP)
pthread_cleanup_push_defer_np (routine, arg);
// now back in deferred cancellation mode
err = pthread_mutex_lock (&mutex);
if (err)
print_error (err, "pthread_mutex_lock", __LINE__);
mcount++;
execute = !err;
// here we go back to async mode and THEN unlock, too late
pthread_cleanup_pop_restore_np (execute);
#else
int oldtype;
pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, &oldtype);
pthread_cleanup_push(routine, arg);
// now back in deferred cancellation mode
err = pthread_mutex_lock (&mutex);
if (err)
print_error (err, "pthread_mutex_lock", __LINE__);
mcount++;
execute = !err;
pthread_cleanup_pop(execute);
pthread_setcanceltype(oldtype, NULL);
#endif
}
}
/*
* 1. launch a child thread
* the child switches to async cancellation mode
* the child sleeps 1s
* the child starts cycling forever taking a mutex and releasing it
* again as fast as it can, using a pthread_cleanup_push_defer_np
* and pthread_cleanup_pop_restore_np pair to protect the
* lock/unlock.
* 2. sleep 2s second
* 3. start sending cancel messages to the child in a loop every 1ms.
*/
int
main ()
{
pthread_t tid;
int err;
beginning:
mutex = (pthread_mutex_t) PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP;
err = pthread_create (&tid, NULL, tfn, NULL);
if (err) {
print_error (err, "pthread_create", __LINE__);
return err;
}
tcount++;
DEBUG ("parent created thread OK\n");
err = pthread_detach (tid);
if (err) {
print_error (err, "pthread_detach", __LINE__);
return err;
}
DEBUG ("parent detached thread OK\n");
DEBUG ("parent sleeps 2s\n");
sleep (2);
DEBUG ("parent wakes after sleep\n");
DEBUG ("parent starts sending cancels to child\n");
for (;; kcount++) {
struct timespec req = { 0, 1000000, }, rem; // 1ms
nanosleep (&req, &rem);
err = pthread_cancel (tid);
switch (err) {
case 0:
// the thread is still alive
break;
case ESRCH:
// the thread died .. test the lock to see if it's set or not
// Set means the thread died without unlocking it.
DEBUG ("parent spots child died\n");
err = pthread_mutex_trylock (&mutex);
switch (err) {
case EBUSY: // mutex is locked + thread is dead = bad!
fprintf (stderr, "!!child died leaving lock set!!\n");
fprintf (stderr, " %d threads created\n", tcount);
fprintf (stderr, " %d cancel signals sent\n", kcount);
fprintf (stderr, " %d lock cycles completed\n", mcount);
return 0;
case 0:
// we got the lock OK, thread dead, try again
err = pthread_mutex_unlock (&mutex);
if (err)
print_error (err, "pthread_mutex_unlock", __LINE__);
DEBUG ("parent sees child cleaned up OK, restarting\n");
goto beginning;
default:
// something else invalid about the trylock attempt
print_error (err, "pthread_mutex_trylock", __LINE__);
return err;
}
break;
default:
print_error (err, "pthread_cancel", __LINE__);
return err;
}
}
// never reached
}
Reply to: