Re: LinuxThreads cancellation bug.

[Kaz Kylheku <kaz at ashi dot footprints dot net>]

On 6 Nov 2000, Ulrich Drepper wrote:

> Kaz Kylheku <kaz@ashi.footprints.net> writes:
> 
> > If cancelation is disabled the pthread_cancel function bails without
> > recording that a cancelation request was made. This is incorrect
> > behavior, because the application cannot temporarily disable
> > cancelation around code without potentially losing a cancelation
> > request.
> 
> I agree with this.
> 
> > I believe that the fix is simply to remove the test.
> 
> This surely isn't true.  Instead we probably should move

I believe that it is sufficient, simply because that extrication pointer is not
registered if the thread's cancelation is disabled, so kill() will be called
rather than restart(). The thread's signal handler will note that cancelation
is disabled and not do anything.

The behavior prior to the extrication stuff addded 2.1.3 was to call kill()
unconditionally, and let the thread's signal handler deal with the cancelation
type and state. I believe that this behavior can still be relied upon, as
before, however ...

>   th->p_canceled = 1;
> 
> before the test.  The next cancelstate change will then have the
> desired effect.

... I don't see anything wrong with this fix at all, so I say just keep that.
This version doesn't require a paragraph's worth of explanation as to why the
function isn't bothering to check for disabled cancelation, and it elides
some useless kill() calls.

But you did say that th->p_canceled = 1 should be *before* the test. I see
the change that you made in CVS is to have it after the test:

  if (th->p_cancelstate == PTHREAD_CANCEL_DISABLE || th->p_canceled) {
    /* 1 */
    th->p_canceled = 1;
    __pthread_unlock(&handle->h_lock);
    return 0;
  }

which I don't quite agree with because the thread could re-enable its
cancelation state at point 1, and thus miss the request until the next
cancelation point. 

I'm thinking of the case in which a thread disables its cancelation briefly,
then reenables it and subsequently never executes a cancelation point until its
termination. Suppose it's a thread which does a lot of lengthy computation
which can be canceled by async cancellation. So the thread some computation,
then disables cancelation and does something, then reenables cancelation and
continues the lengthy computation. With the above fix, it could miss the one
and only cancelation request.

How about something like:

    int already_canceled;

    /*...*/

    already_canceled = th->p_canceled;
    th->p_canceled = 1;

    if (already_canceled || th->p_cancelstate == PTHREAD_CANCEL_DISABLE) {
      __pthread_unlock(&handle->h_lock);
      return 0;
    }