Bug#459322: libc6-dev: implementation of pthread_cleanup_pop_restore_np(1) is bogus

["Peter T. Breuer" <ptb@inv.it.uc3m.es>]

Package: libc6-dev
Version: 2.7-4
Severity: normal


pthread_cleanup_pop_restore_np(1) is implemented as ..

   } while (0);
       __pthread_unregister_cancel_restore (&__cancel_buf);
           if (1)
                   __cancel_routine (__cancel_arg);
                       } while (0)


and that means the cancel routine may not get executed if the thread
dies after the unregister but before the execution of the cancel
routine.

Thus for example, if pthread_mutex_unlock is the registered cancellation
routine, it will never get executed, leaving the thread group's mutex locked,
and locking out other threads.

This seems to me to contradict what the manual page (for
pthread_cleanup_push) says:

    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 ini- tially in asynchronous cancellation
    mode.

but who knows what jargon like "cancellation type" and "deferred" means.
Please use plain english. If you mean "die" by "cancellation" (with
slight nuances about whether a condemned thread is dead yet or not), and
you mean that it won't wait to join back to the main thread from which
it detached by "asynchronous cancellation mode", then maybe there is
some universe in which the interpretation of those words means something
compatible with the behaviour. But to me, if it dies, it can leave the
lock hanging.

And in any case, it's all silly the way it is ..  even as a macro, you
should just set up the data you are saving as a declaration

   pthread_save_class mydata;

and then have your push and pop routines refer to it explicitly

   pthread_push_whatever(&mydata, myfn, myarg);

   ...
   pthread_pop_whatever(&mydata, 1);

then we could stop playing with those hidden open and close braces!

As it is they are arguably wrongheaded as is, because one has to put the
pop in a labelled place at the end of a routine

   pop:
   pthread_pop_whatever(1);

and that won't work, because we have a label at the end of a block that
way, thanks to your do{ ///////  } while(0) stuff.

One has to put the pop at the end, with goto pop; as one can't lock and
unlock mutexes half way through a routine, in a while loop, because of
your extra braces. It wrecks the loops.

Sigh .. and the man page gets the name of the push routine wrong, at the
end:

  
                pthread_cleanup_push_restore_np(pthread_mutex_unlock, (void *) &mut);
                               ^^^^^^^^^^^^^ arrgh.
                pthread_mutex_lock(&mut);
                /* do some work */
                pthread_cleanup_pop_restore_np(1);

Arguably, that name would be more sensible!


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-4      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