Bug#459322: libc6-dev: implementation of pthread_cleanup_pop_restore_np
Package: libc6-dev
Version: 2.7-5
Followup-For: Bug #459322
The current implementation of implementation of
pthread_cleanup_pop_restore_np in pthread.h apparently has the two
function calls it makes in the wrong order. The way round it is now
only works if the thread was in DEFERRED cancellation mode before the
push, not if the thread was in ASYNCHRONOUS cancellation mode at that
time. The whole point of pthread_cleanup_pop_restore_np (and the
corresponding push) is that it should work in ASYNCHRONOUS mode ..
otherwise we would have used simple pthread_cleanup_pop (and push). I
quote the manpage for pthread_cleanup_push_defer_np on that point:
pthread_cleanup_push_defer_np is a non-portable extension that
combines pthread_cleanup_push and pthread_setcanceltype(3). It pushes
a cleanup handler just as pthread_cleanup_push does, but also saves
the current cancellation type and sets it to deferred cancellation.
This ensures that the cleanup mechanism is effective even if the
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
thread was initially in asynchronous cancellation mode.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But the implementation apparently fails to honour that promise. Here's
a patch:
--- pthread.h.orig 2008-01-05 21:48:50.000000000 +0100
+++ pthread.h 2008-01-05 21:54:57.000000000 +0100
@@ -656,9 +656,9 @@ extern void __pthread_register_cancel (_
If EXECUTE is non-zero, the handler function is called. */
# define pthread_cleanup_pop(execute) \
} while (0); \
- __pthread_unregister_cancel (&__cancel_buf); \
if (execute) \
__cancel_routine (__cancel_arg); \
+ __pthread_unregister_cancel (&__cancel_buf); \
} while (0)
extern void __pthread_unregister_cancel (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute;
@@ -691,9 +691,9 @@ extern void __pthread_register_cancel_de
pthread_cleanup_push_defer was called. */
# define pthread_cleanup_pop_restore_np(execute) \
} while (0); \
- __pthread_unregister_cancel_restore (&__cancel_buf); \
if (execute) \
__cancel_routine (__cancel_arg); \
+ __pthread_unregister_cancel_restore (&__cancel_buf); \
} while (0)
extern void __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute;
The order in pthread.h as it was originally is
1) __pthread_unregister_cancel (&__cancel_buf);
2) __cancel_routine (__cancel_arg);
Unfortunately, (1) returns us to ASYNCHRONOUS cancellation mode, if we
were in it before the push, so a cancellation can happen anytime,
according to the man page, including before (2), thus preempting
the running of the intended cancellation routine. The manpage for
pthread_cancel says:
cancellation type: either PTHREAD_CANCEL_ASYNCHRONOUS to cancel the
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
calling thread as soon as the cancellation request is received, or
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
PTHREAD_CANCEL_DEFERRED to keep the cancellation request pending
until the next cancellation point
so I presume that cancellation in the ASYNCHRONOUS mode can happen
between (1) and (2), thus defeating the present implementation. I'd
be delighted to hear if it weren't true ... and more delighted to
hear that signals can't cause thread death there either.
Anyway, the proposed patch makes a wild guess at what is the intended
code, and restores it to that. Feel free to fix it. The C++ needs
looking at too. I haven't so much as glanced at it. Ah ... but I have
now, and it also looks backwards to me. Try this:
@@ -691,9 +691,9 @@ extern void __pthread_register_cancel_de
pthread_cleanup_push_defer was called. */
# define pthread_cleanup_pop_restore_np(execute) \
} while (0); \
- __pthread_unregister_cancel_restore (&__cancel_buf); \
if (execute) \
__cancel_routine (__cancel_arg); \
+ __pthread_unregister_cancel_restore (&__cancel_buf); \
} while (0)
extern void __pthread_unregister_cancel_restore (__pthread_unwind_buf_t *__buf)
__cleanup_fct_attribute;
This stuff must be near impossible to test. I'd be interested in
getting a copy of your test harness.
The manpage for pthread_cleanup_pop_restore_np gives a clue that the
thread mode restore is supposed to be AFTER running the cleanup function
in the final paras:
If the code above must also work in asynchronous cancellation mode,
then it must switch to deferred mode for locking and unlocking the
mutex:
pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, &oldtype);
pthread_cleanup_push(pthread_mutex_unlock, (void *) &mut);
pthread_mutex_lock(&mut);
/* do some work */
pthread_cleanup_pop(1);
pthread_setcanceltype(oldtype, NULL);
and there we see the restore of the oldtype (cancellation mode) after
the cleanup function has run (in pthread_cleanup_pop). The next paragraph
claims the above is equivalent to
The code above can be rewritten in a more compact and more efficient
way, using the non-portable functions pthread_cleanup_push_defer_np and
pthread_cleanup_pop_restore_np:
pthread_cleanup_push_restore_np(pthread_mutex_unlock, (void *) &mut);
pthread_mutex_lock(&mut);
/* do some work */
pthread_cleanup_pop_restore_np(1);
But it isn't, at present. The patch above is intended to correct that.
Peter
-- System Information:
Debian Release: lenny/sid
APT prefers testing
APT policy: (500, 'testing')
Architecture: i386 (i686)
Kernel: Linux 2.6.15.3 (PREEMPT)
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968) (ignored: LC_ALL set to C)
Shell: /bin/sh linked to /bin/bash
Versions of packages libc6-dev depends on:
ii libc6 2.7-5 GNU C Library: Shared libraries
ii linux-libc-dev 2.6.22-6 Linux Kernel Headers for developme
Versions of packages libc6-dev recommends:
ii gcc [c-compiler] 4:4.2.1-6 The GNU C compiler
ii gcc-2.95 [c-compile 1:2.95.4-27 The GNU C compiler
ii gcc-3.4 [c-compiler 3.4.6-6 The GNU C compiler
ii gcc-4.1 [c-compiler 4.1.2-18 The GNU C compiler
ii gcc-4.2 [c-compiler 4.2.2-4 The GNU C compiler
ii tcc [c-compiler] 0.9.24~cvs20070502-2 the smallest ANSI C compiler
-- no debconf information
Reply to: