Re: [perl #81074] Perl reports SIGINT handler "IGNORE" not defined inmulti-threaded apps

On Mon, Jul 28, 2014 at 6:15 AM, Tony Cook via RT <perlbug-followup@perl.org
> wrote:

> --- a/mg.c
> +++ b/mg.c
> @@ -1353,6 +1353,15 @@ Perl_csighandler(int sig)
>      PERL_UNUSED_ARG(sip);
>      PERL_UNUSED_ARG(uap);
>  #endif
> +#ifdef USE_ITHREADS
> +    /* handle a signal received in a non-perl thread, eg. a thread
> +       created by a framework */
> +    if (!my_perl) {
> +        my_perl = PL_curinterp;
> +        PERL_SET_THX(my_perl);
> +    }
> +#endif
> +
>  #ifdef FAKE_PERSISTENT_SIGNAL_HANDLERS
>      (void) rsignal(sig, PL_csighandlerp);
>      if (PL_sig_ignoring[sig]) return;
>

You're peeking and poking into an interpreter owned by another thread. This
is suffering from a race condition. If possible, a pthread_kill to the main
thread would be preferable I guess.


> @@ -1384,16 +1393,33 @@ Perl_csighandler(int sig)
>         (*PL_sighandlerp)(sig);
>  #endif
>      else {
> +        int pending;
>         if (!PL_psig_pend) return;
>         /* Set a flag to say this signal is pending, that is awaiting
> delivery after
>          * the current Perl opcode completes */
> -       PL_psig_pend[sig]++;
> +#ifdef USE_ITHREADS
> +        /* Only deliver to the current thread if it has a non-default
> +           signal handler set */
> +        if (PL_sighand_set && PL_sighand_set[sig]) {
> +            PL_psig_pend[sig]++;
> +            pending = ++PL_sig_pending;
> +        }
> +        else {
> +            /* no handler specific to thread, deliver to the main thread
> */
> +            dTHXa(PL_curinterp);
> +            PL_psig_pend[sig]++;
> +            pending = ++PL_sig_pending;
> +        }
>

The else block is racy too, though over a smaller scope than the previous
one.


> +
> +#ifdef USE_ITHREADS
> +    if (!isGV_with_GP(sig_ptr) && !SvROK(sig_ptr) && SvOK(sig_ptr)) {
> +        /* handle a thread receiving a signal it isn't expecting,
> +           there's a few ways this can occur, see [perl #81074] and
> +           [perl #120951] for some examples and discussion.
> +        */
> +        STRLEN siglen;
> +        const char *signame = SvPV_const(sig_ptr, siglen);
> +        if (memEQs(signame, siglen, "IGNORE")) {
> +            if (aTHX != PL_curinterp) {
> +                dTHXa(PL_curinterp);
> +                PL_psig_pend[sig]++;
> +                ++PL_sig_pending;
> +            }
> +            /* else drop it on the floor, since it's ignored */
> +
> +            goto cleanup;
> +        }
>

I don't see the logic of sending a signal to the main thread when we asked
for it to be ignored. We should probably just ignore it instead. Not that
there is a foolproof way of handling this.


> +        else if (memEQs(signame, siglen, "DEFAULT")) {
> +            if (aTHX == PL_curinterp) {
> +                /* what to do here?  The user is probably doing
> +                   something fairly strange or advanced, beat them
> +                   with a stick */
> +                PerlIO_printf(Perl_error_log, "Signal SIG%s received with
> DEFAULT signal handler set.\n",
> +                              PL_sig_name[sig]);
> +                exit(sig);
> +            }
>

I can think of at least one race condition that could cause that, but that
has existed for over 20 years and doesn't seem to be much of a deal.


> +            else {
> +                /* hopefully the main thread knows what to do with it */
> +                dTHXa(PL_curinterp);
> +                PL_psig_pend[sig]++;
> +                ++PL_sig_pending;
> +
> +                goto cleanup;
> +            }
> +        }
> +    }
> +#endif
> +
>

Again, this is racy for the previously mentioned reasons.


Also, it should be noted that your additions feature is enabled on
USE_ITHREADS, even though your solution is multiplicity based and the
problem is also a multiplicity issue. Then again, the solution I suggested
for your race condition is assuming ithreads. I'm not sure how to deal with
that, possibly by not caring.

Leon

• [perl #81074] Perl reports SIGINT handler "IGNORE" not defined in multi-threaded apps by Tony Cook via RT
• Re: [perl #81074] Perl reports SIGINT handler "IGNORE" not defined inmulti-threaded apps by Leon Timmermans
• [perl #81074] Perl reports SIGINT handler "IGNORE" not defined in multi-threaded apps by bulk88 via RT
• Re: [perl #81074] Perl reports SIGINT handler "IGNORE" not defined inmulti-threaded apps by Leon Timmermans