[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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

Your message dated Sat, 05 Jan 2008 17:09:30 +0100
with message-id <20080105160930.GB22996@artemis.madism.org>
and subject line Bug#459322: libc6-dev: implementation of pthread_cleanup_pop_restore_np(1) is bogus
has caused the attached Bug report to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
Bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what I am
talking about this indicates a serious mail system misconfiguration
somewhere.  Please contact me immediately.)

Debian bug tracking system administrator
(administrator, Debian Bugs database)
--- Begin Message --- 
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

--- End Message ---
--- Begin Message --- 
On Sat, Jan 05, 2008 at 02:44:36PM +0000, Peter T. Breuer wrote:
> 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.

  Wrong, because __pthread_unregister_cancel_restore isn't a
cancellation point.

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

  That's wrong.

> but who knows what jargon like "cancellation type" and "deferred" means.

  Well cancellation point, and deferred are specific terms with very
precise meanings in the pthread context, please read POSIX about that (I
believe manpages-posix-dev or sth like that in non-free has the proper
man pages). If you don't get those concepts, you can't understand how
pthread_cleanup_pop_restore_np works.

  And yes, threads are _complex_ you cannot grok it only reading a few
manpages.

> 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

  That's because it works the very same way pthread_cleanup_push and
pthread_cleanup_pop work, and those functions are POSIX, and POSIX says
that those functions _MUST_ be used at the same context level, and
always must be used together. It's how it works, and glibc won't change
it, because it's what the norm says about portable code.

  I'll skip the rest of this bug report that is mostly bogus because you
made wrong assumptions.

Cheers,
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

Attachment: pgpkz9VSd08r5.pgp
Description: PGP signature


--- End Message ---
Reply to: