Skip Menu |

This queue is for tickets about the DBI CPAN distribution.

Report information
The Basics
Id: 50621
Status: resolved
Priority: 0/
Queue: DBI
People
Owner: Nobody in particular
Requestors: eponymousalias [...] yahoo.com
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Sun Oct 18 15:50:31 2009 eponymousalias [...] yahoo.com - Ticket created
Subject: Signal-handling sample code is buggy
Date: Sun, 18 Oct 2009 12:50:04 -0700 (PDT)
To: bug-DBI [...] rt.cpan.org
From: eponymous alias <eponymousalias [...] yahoo.com>
The example code for signal handling: eval { local $SIG{ALRM} = sub { die "TIMEOUT\n" }; alarm($seconds); ... code to execute with timeout here ... alarm(0); # cancel alarm (if code ran fast) }; alarm(0); # cancel alarm (if eval failed) if ( $@ eq "TIMEOUT\n" ) { ... } is buggy. Suppose the eval dies for some reason unrelated to the signal handling just before the alarm expires, and then the code exits the eval, and then the alarm expires before the final alarm(0) can be called. Now either the code will completely die because there is no $SIG{ALRM} handler in place to catch the signal, or the wrong handler (not the local handler) will be called. What you need instead is: eval { local $SIG{ALRM} = sub { die "TIMEOUT\n" }; eval { alarm($seconds); ... code to execute with timeout here ... alarm(0); # cancel alarm (if code ran fast) }; alarm(0); # cancel alarm (if eval failed) die $@ if $@; }; if ( $@ eq "TIMEOUT\n" ) { ... } so the desired signal handler is called in this situation. The same problem affects the sample sigaction code just below this timeout example.
  Mon Oct 19 04:30:44 2009 Tim.Bunce [...] pobox.com - Correspondence added
Subject: Re: [rt.cpan.org #50621] Signal-handling sample code is buggy
Date: Mon, 19 Oct 2009 09:30:20 +0100
To: eponymous alias via RT <bug-DBI [...] rt.cpan.org>
From: Tim Bunce <Tim.Bunce [...] pobox.com>
On Sun, Oct 18, 2009 at 03:50:32PM -0400, eponymous alias via RT wrote: Show quoted text
> The example code for signal handling: [...] > What you need instead is: [...] > > The same problem affects the sample sigaction code just below > this timeout example.
Thanks! Could you send me a patch to fix both? Tim. p.s. I think the "alarm(0); # cancel alarm (if code ran fast)" isn't needed as the following alarm(0), just after the inner eval, will work just as well and it doesn't matter if a SIGALRM arrives before it's executed.
  Mon Oct 19 04:30:45 2009 The RT System itself - Status changed from 'new' to 'open'
  Sun Jan 17 10:08:59 2010 lab [...] lincolnbaxter.com.MAKE.ME.VALID - Correspondence added
RT-Send-CC: Tim.Bunce [...] pobox.com
On Mon Oct 19 04:30:44 2009, Tim.Bunce@pobox.com wrote: Show quoted text
> On Sun, Oct 18, 2009 at 03:50:32PM -0400, eponymous alias via RT wrote: > p.s. I think the "alarm(0); # cancel alarm (if code ran fast)" > isn't needed as the following alarm(0), just after the inner eval, > will work just as well and it doesn't matter if a SIGALRM arrives > before it's executed.
Tim, I just noticed this yesterday. Not sure why I missed it when it was first entered. I see the suggested fix is wrapping the eval with an eval. I wonder if this bug was ever actually manifested in as system, the submitter owns or manages, logically I think he is right, but given the fact that we are essentially revert to "unsafe" signal handling here, and we still don't have a proposed fix to the safe flags problem I have identified in my tests, I wonder if it really matters. The code is somewhat ugly. If I understand your remark correctly, I think you are saying (and I think I agree) that that inner cancel is unnecessary. I just looked in the DBI pod to see if your example code was ever patched... Did you ever get a patch, that you have not released yet? I'm going to think about this for a day or too. I don't think this impacts the Sys::SigAction code itself, though it would require changes to the tests. I'll modify the tests, and if no negative impacts are found (i expect not), I'll probably update Sys::SigAction in the next week, with the modified tests and POD updates. Has this been updated this in DBI?, or if not, would you like a patch from me, that is consistent with what I finally do with Sys::SigAction? Lincoln
  Tue Jan 19 03:05:30 2010 eponymousalias [...] yahoo.com - Correspondence added
Subject: Re: [rt.cpan.org #50621] Signal-handling sample code is buggy
Date: Tue, 19 Jan 2010 00:05:03 -0800 (PST)
To: bug-DBI [...] rt.cpan.org
From: eponymous alias <eponymousalias [...] yahoo.com>
--- On Sun, 1/17/10, Lincoln A Baxter via RT <bug-DBI@rt.cpan.org> wrote: Show quoted text
> I wonder if this bug was ever actually manifested in a > system, the submitter owns or manages,
No, I spotted the bug not by exercising it, but by long experience in staring down uncommon conditions, as I was reading the documentation. But the bug could be simulated easily enough, by a "die" right after alarm($seconds) and introducing an infinite loop (just to demonstrate what would otherwise be a rare condition) right before the second alarm(0). Show quoted text
> Logically I think he is right, but given the fact that > we are essentially revert to "unsafe" signal handling > here, and we still don't have a proposed fix to the safe > flags problem I have identified in my tests, I wonder if > it really matters.
I would suggest that you read page 417 of Programming Perl, 3/e for the basic idea. However, the code presented there is simply wrong, and suffers from the same condition of having the local ALRM handler out of scope when the alarm might finally occur just before the second "alarm 0;". My copy of the book has a handwritten note "code is wrong -- see errata", but the O'Reilly site is (ridiculously) showing no confirmed errata for this book. I believe it used to, as that would explain my note, but the errata page has not been properly maintained. Also, the explanation in the last two sentences of the page is clearly bogus, though it does hint at the actual problem. The authors had the right intuition but failed to capture reality in both their implementation and their explanation. Show quoted text
> The code is somewhat ugly. Did you ever get a patch, > that you have not released yet?
Sorry, I've been too busy (writing other code, and finding bugs in other Perl-modules, some submitted and some still queued up) to submit a patch.
  Wed Mar 10 08:37:32 2010 bohica [...] ntlworld.com - Correspondence added
On Tue Jan 19 03:05:30 2010, eponymousalias@yahoo.com wrote: Show quoted text
> --- On Sun, 1/17/10, Lincoln A Baxter via RT <bug-DBI@rt.cpan.org> wrote:
> > I wonder if this bug was ever actually manifested in a > > system, the submitter owns or manages,
> > No, I spotted the bug not by exercising it, but by long > experience in staring down uncommon conditions, as I was > reading the documentation. But the bug could be simulated > easily enough, by a "die" right after alarm($seconds) and > introducing an infinite loop (just to demonstrate what > would otherwise be a rare condition) right before the > second alarm(0). >
> > Logically I think he is right, but given the fact that > > we are essentially revert to "unsafe" signal handling > > here, and we still don't have a proposed fix to the safe > > flags problem I have identified in my tests, I wonder if > > it really matters.
> > I would suggest that you read page 417 of Programming Perl, > 3/e for the basic idea. However, the code presented there > is simply wrong, and suffers from the same condition of > having the local ALRM handler out of scope when the alarm > might finally occur just before the second "alarm 0;". My > copy of the book has a handwritten note "code is wrong -- > see errata", but the O'Reilly site is (ridiculously) showing > no confirmed errata for this book. I believe it used to, as > that would explain my note, but the errata page has not been > properly maintained. Also, the explanation in the last two > sentences of the page is clearly bogus, though it does hint > at the actual problem. The authors had the right intuition > but failed to capture reality in both their implementation > and their explanation. >
> > The code is somewhat ugly. Did you ever get a patch, > > that you have not released yet?
> > Sorry, I've been too busy (writing other code, and finding > bugs in other Perl-modules, some submitted and some still > queued up) to submit a patch. >
The example from the documentation modified to demonstrate the problem as the OP reported is: use strict; use warnings; $SIG{ALRM} = sub {print "outer alarm handler\n"}; eval { local $SIG{ALRM} = sub { die "TIMEOUT\n" }; alarm(5); die "fred"; # long running code that dies before alarm }; # infinite loop to demonstrate what happens if alarm(0) is not # called in time while (1) { sleep 1; } alarm(0); # cancel alarm (if eval failed) if ( $@ eq "TIMEOUT\n" ) { print "timeout seen\n"; } produces: $ ~/svn/dbi/trunk$ perl ex/timeout.pl outer alarm handler The corrected example (which I'll document) and modified to demonstrate the problem is: use strict; use warnings; $SIG{ALRM} = sub {print "outer alarm handler\n"}; eval { local $SIG{ALRM} = sub { die "TIMEOUT\n" }; eval { alarm(5); die "fred"; # long running code that dies before alarm }; alarm(0); die "$@\n" if $@; }; while (1) { sleep 1; } if ( $@ eq "TIMEOUT\n" ) { print "timeout seen\n"; } elsif ($@) { print "died with $@\n"; } which produces no output (correctly). I'll update both examples in subversion. Martin -- Martin J. Evans Wetherby, UK
  Wed Mar 10 12:43:15 2010 Tim.Bunce [...] pobox.com - Correspondence added
Subject: Re: [rt.cpan.org #50621] Signal-handling sample code is buggy
Date: Wed, 10 Mar 2010 15:15:31 +0000
To: Martin J Evans via RT <bug-DBI [...] rt.cpan.org>
From: Tim Bunce <Tim.Bunce [...] pobox.com>
On Wed, Mar 10, 2010 at 08:37:33AM -0500, Martin J Evans via RT wrote: Show quoted text
> > eval { > local $SIG{ALRM} = sub { die "TIMEOUT\n" }; > eval { > alarm(5); > die "fred"; # long running code that dies before alarm > };
maybe add an explanatory comment: # outer eval catches alarm that might fire JUST before this alarm(0) Show quoted text
> alarm(0); > die "$@\n" if $@;
just: die $@ if $@; to avoid adding an additional newline to $@ which would break the $@ eq "TIMEOUT\n" check below. Show quoted text
> I'll update both examples in subversion.
Thanks Martin! Tim.
  Fri Apr 30 08:18:05 2010 bohica [...] ntlworld.com - Correspondence added
On Wed Mar 10 12:43:15 2010, Tim.Bunce@pobox.com wrote: Show quoted text
> On Wed, Mar 10, 2010 at 08:37:33AM -0500, Martin J Evans via RT wrote:
> > > > eval { > > local $SIG{ALRM} = sub { die "TIMEOUT\n" }; > > eval { > > alarm(5); > > die "fred"; # long running code that dies before alarm > > };
> > maybe add an explanatory comment: > > # outer eval catches alarm that might fire JUST before this alarm(0) >
> > alarm(0); > > die "$@\n" if $@;
> > just: die $@ if $@; to avoid adding an additional newline to $@ > which would break the $@ eq "TIMEOUT\n" check below. >
> > I'll update both examples in subversion.
> > Thanks Martin! > > Tim.
For reason I missed Tim's last comment on this RT and have just checked in the change. I think this rt can be written off now. Martin -- Martin J. Evans Wetherby, UK
  Fri Apr 30 16:42:52 2010 TIMB [...] cpan.org - Correspondence added
Resolved by r13940
  Fri Apr 30 16:42:53 2010 TIMB [...] cpan.org - Status changed from 'open' to 'resolved'