Re: plperl crash with Debian 6 (64 bit), pl/perlu, libwww and https

[I've included a summary of the thread and Bcc'd this to perl5-porters
for a sanity check. Please trim heavily when replying.]

On Thu, Aug 04, 2011 at 09:42:31AM -0400, Andrew Dunstan wrote:
>
> So doesn't this look like a bug in the perl module that sets the
> signal handler and doesn't restore it?
>
> What happens if you wrap the calls to the module like this?:
>
> {
> local $SIG{ALRM};
> # do LWP stuff here
> }
> return 'OK';
>
> That should restore the old handler on exit from the block.
>
> I think if you use a perl module that monkeys with the signal
> handlers for any signal postgres uses all bets are off.

On Thu, Aug 04, 2011 at 10:28:45AM -0400, Tom Lane wrote:
>
> > Sure, but how expensive would it be for pl/perl to do this
> > automatically ?
>
> How can anything like that possibly work with any reliability
> whatsoever? If the signal comes in, you don't know whether it was
> triggered by the event Postgres expected, or the event the perl module
> expected, and hence there's no way to deliver it to the right signal
> handler (not that the code you're describing is even trying to do that).
>
> What *I'd* like is a way to prevent libperl from touching the host
> application's signal handlers at all. Sadly, Perl does not actually
> think of itself as an embedded library, and therefore thinks it owns all
> resources of the process and can diddle them without anybody's
> permission.

The PERL_IMPLICIT_SYS mechanism addresses this. Unfortunately it only
works with USE_ITHREADS on Windows currently.
http://perldoc.perl.org/perlguts.html#Future-Plans-and-PERL_IMPLICIT_SYS

On Thu, Aug 04, 2011 at 04:09:47PM -0600, Alex Hunsaker wrote:
>
> Well we can't prevent perl XS (aka C) from messing with signals (and
> other modules like POSIX that expose things like sigprocmask,
> siglongjump etc.) , but we could prevent plperl(u) from playing with
> signals on the perl level ala %SIG.
>
> [ IIRC I proposed doing something about this when we were talking
> about the whole Safe mess, but I think there was too much other
> discussion going on at the time :-) ]
>
> Mainly the options im thinking about are:
> 1) if anyone touches %SIG die
> 2) turn %SIG into a regular hash so people can set/play with %SIG, but
> it has no real effect.
> 3) local %SIG before we call their trigger function. This lets signals
> still work while "in trigger scope" (like we do for %_TD)
> 4) if we can't get any of the above to work we can save each %SIG
> handler before and restore them after each trigger call. (mod_perl
> does something similar so Im fairly certain we should be able to get
> that to work)

On Thu, Aug 4, 2011 at 16:34, David E. Wheeler <david(at)kineticode(dot)com> wrote:
>
>> 1) if anyone touches %SIG die
>> 2) turn %SIG into a regular hash so people can set/play with %SIG, but
>> it has no real effect.
>
> These would disable stuff like $SIG{__WARN__} and $SIG{__DIE__}, which would be an unfortunate side-effect.

On Thu, Aug 04, 2011 at 07:52:45PM -0400, Tom Lane wrote:
>
> The scenario I was imagining was:
>
> 1. perl temporarily takes over SIGALRM.
>
> 2. while perl function is running, statement_timeout expires, causing
> SIGALRM to be delivered.
>
> 3. perl code is probably totally confused, and even if it isn't,
> statement_timeout will not be enforced since Postgres won't ever get the
> interrupt.
>
> Even if you don't think statement_timeout is a particularly critical
> piece of functionality, similar interference with the delivery of, say,
> SIGUSR1 would be catastrophic.
>
> How do you propose to prevent this sort of problem?

I don't think there's complete solution for that particular scenario.
[Though redirecting the perl alarm() opcode to code that would check the
argument against the remaining seconds before statement_timeout expires,
might get close.]

On Thu, Aug 04, 2011 at 06:44:18PM -0600, Alex Hunsaker wrote:
>
> > How do you propose to prevent this sort of problem?
>
> Well, I think that makes it unworkable.
>
> So back to #1 or #2.
>
> For plperlu sounds like we are going to need to disallow setting _any_
> signals (minus __DIE__ and __WARN__). I should be able to make it so
> when you try it gives you a warning something along the lines of
> "plperl can't set signal handlers, ignoring...".
>
> For plperl I think we should probably do the same. It seems like
> Andrew might disagree though? Anyone else want to chime in on if
> plperl lets you muck with signal handlers?
>
> Im not entirely sure how much of this is workable, I still need to go
> through perl's guts and see. At the very worst I think we can mark
> each signal handler that is exposed in %SIG readonly (which would mean
> we would die instead of warning), but I think I can make the warning
> variant workable as well.
>
> I also have not dug deep enough to know how to handle __WARN__ and
> __DIE__ (and exactly what limitations allowing those will impose). I
> still have some work at $day_job before I can really dig into this.

On Thu, Aug 04, 2011 at 09:40:57PM -0400, Andrew Dunstan wrote:
>
> Let's slow down a bit. Nobody that we know of has encountered the
> problem Tom's referring to, over all the years plperlu has been
> available. The changes you're proposing have the potential to
> downgrade the usefulness of plperlu considerably without fixing
> anything that's known to be an actual problem. Instead of fixing a
> problem caused by using LWP you could well make LWP totally unusable
> from plperlu.
>
> And it still won't do a thing about signal handlers installed by C code.
>
> And plperlu would be the tip of the iceberg. What about all the
> other PLs, not to mention non-PL loadable modules?
>
> But we *can* fix the original problem reported, namely failure to
> restore signal handlers on function exit, with very little downside
> (assuming it's shown to be fairly cheap).

On Thu, Aug 04, 2011 at 09:23:49PM -0600, Alex Hunsaker wrote:
>
> > And plperlu would be the tip of the iceberg. What about all the other PLs,
> > not to mention non-PL loadable modules?
>
> Maybe the answer is to re-issue the appropriate pqsignals() instead of
> doing the perl variant?
>
> For PL/Perl(u) we could still disallow any signals the postmaster
> uses, from my quick look that would be: HUP, INT, TERM, QUIT, ALRM,
> PIPE, USR1, USR2, FPE. All we would need to do is restore ALRM.
>
> Or am I too paranoid about someone shooting themselves in the foot via
> USR1? (BTW you can set signals in plperl, but you can't call alarm()
> or kill())

On Fri, Aug 05, 2011 at 10:53:21AM -0400, Andrew Dunstan wrote:
>
> This whole thing is a massive over-reaction to a problem we almost
> certainly know how to fix fairly simply and relatively painlessly,
> and attempts (unsuccessfully, at least insofar as comprehensiveness
> is concerned) to fix a problem nobody's actually reported having
> AFAIK.

For plperl, as Alex noted above, kill() and alarm() can't be used but
%SIG can be altered. Locally making %SIG readonly for plperl subs
(after __DIE__ and __WARN__ are added) seems cheap and effective.

For plperlu, clearly $SIG{ALRM} is useful. Enforcing localization, thus
fixing the immediate problem, and documenting that it won't work
reliably with statement_timeout, seems like a reasonable approach.

plperlu is already a potential footgun in countless ways. Documenting
that other signal handlers, like USR1, shouldn't be used ought to be enough.

On Sat, Aug 06, 2011 at 12:37:28PM -0600, Alex Hunsaker wrote:
>
> *shrug* OK.
>
> Find attached a version that does the equivalent of local %SIG for
> each pl/perl(u) call.

> + gv = gv_fetchpv("SIG", 0, SVt_PVHV);
> + save_hash(gv); /* local %SIG */

After a little digging and some discussion on the #p5p channel [thanks
to ilmari++ leont++ and sorear++ for their help] it seems that local(%SIG)
doesn't do what you might expect. The %SIG does become empty but the OS
level handlers, even those installed by perl, *aren't changed*:

$ perl -wE '$SIG{INT} = sub { say "Foo"}; { local %SIG; kill "INT", $$; };'
Foo

And, even worse, they're not reset at scope exit:

$ perl -wE '$SIG{INT} = sub { say "Foo"}; { local %SIG; $SIG{INT} = sub {say "Bar" }} kill "INT", $$;'
Bar

That sure seems like a bug (I'll check with the perl5-porters list).

Localizing an individual element of %SIG works fine.
In C that's something like this (untested):

hv = gv_fetchpv("SIG", 0, SVt_PVHV);
keysv = ...SV containing "ALRM"...
he = hv_fetch_ent(hv, keysv, 0, 0);
if (he) { /* arrange to restore existing elem */
save_helem_flags(hv, keysv, &HeVAL(he), SAVEf_SETMAGIC);
}
else { /* arrange to delete a new elem */
SAVEHDELETE(hv, keysv);
}

Tim.