Re: Bug in PoCo::Child when PID wraps round

Hi Erick,

I work with Tony.  I have a new patch now which addresses this issue,  
as well as another leak:  calling $self->wheel($id) will initialize a  
key in $self->{$PKG}{wheels}{$id}, causing a leak if the wheel had  
already finished or if it never existed.

Cheers,
Ricky


=== BEGIN PATCH ===
*** Child.pm	2009-06-02 16:10:53.120660674 +0200
--- Child.pm.patched	2009-05-28 16:38:08.687685140 +0200
***************
*** 265,279 ****

   	# clean up

! 	delete $self->{$PKG}{wheels}{$id};
! 	delete $self->{$PKG}{pids}{$id};

   	# all expiring children should issue a "done" except when
   	# the return code is non-zero which indicates a failure
   	# if the caller asked we quit, fire a "done" regardless
   	# of the child's return code value (we might have hard killed)

! 	my $event = ($self->{$PKG}{wheels}{$id}{quit} || $rc == 0)
   		? "done" : "died"
   		;
   	$self->callback($event, { wheel => $id, rc => $rc });
--- 265,281 ----

   	# clean up

! 	delete $self->{$PKG}{pids}{$self->{$PKG}{wheels}{$id}{ref}->PID};
!         my $wheel = delete $self->{$PKG}{wheels}{$id};
! 	delete $self->{$PKG}{SIGCHLD}{$id};
! 	delete $self->{$PKG}{CLOSED}{$id};

   	# all expiring children should issue a "done" except when
   	# the return code is non-zero which indicates a failure
   	# if the caller asked we quit, fire a "done" regardless
   	# of the child's return code value (we might have hard killed)

! 	my $event = ($wheel->{quit} || $rc == 0)
   		? "done" : "died"
   		;
   	$self->callback($event, { wheel => $id, rc => $rc });
***************
*** 306,311 ****
--- 308,314 ----
   sub wheel {
   	my $self = shift;
   	my $id = shift || $self->{$PKG}{wheels}{current};
+ 	return undef unless exists $self->{$PKG}{wheels}{$id};
   	$self->{$PKG}{wheels}{$id}{ref};
   	}
=== END PATCH ===


On Jun 4, 2009, at 7:30 PM, Erick Calder wrote:

> sorry, I still own it.  I've just been disconnected from the world  
> a long
> time.  it will take me a while to get back to you on this but I will
>
>> From: Tony Wildish <[email protected]>
>> Date: Thu, 7 May 2009 10:58:12 +0200
>> To: <[email protected]>
>> Subject: Re: Bug in PoCo::Child when PID wraps round (fwd)
>>
>> Hi,
>>
>>  just checking one more time: is anyone looking after PoCo::Child,  
>> or is
>> it orphaned?
>>
>>  TIA,
>>  Tony.
>>
>> On Wed, 15 Apr 2009, Tony Wildish wrote:
>>
>>> Hi,
>>>
>>>  I sent this to the [email protected] list some  
>>> time ago
>>> but it looks like that list is dead. Is there someone here managing
>>> PoCo::Child?
>>>
>>>  Cheers,
>>>  Tony.
>>>
>>> ---------- Forwarded message ----------
>>> Date: Tue, 24 Mar 2009 12:56:33 +0100 (CET)
>>> From: Tony Wildish <[email protected]>
>>> To: [email protected]
>>> Subject: Bug in PoCo::Child when PID wraps round
>>>
>>> Hi,
>>>
>>>  we have been using PoCo::Child in a project for a while now and  
>>> have just
>>> uncovered a bug. The internal bookkeeping for the association of  
>>> wheelIDs
>>> and PIDs is wrong, with the result that when the OS PID wraps  
>>> round, the
>>> wrong wheelID will be given to the 'done' event.
>>>
>>>  I'm using PoCo::Child version 1.39, perl 5.8.5, kernel
>>> 2.6.9-78.0.8.EL.cernsmp.
>>>
>>>  You can see the bug by inspection of the code. At line 192 we have:
>>>
>>>         my $id = $wheel->ID;
>>>         $self->debug(qq/run(): "@$cmd", wheel=$id, pid=/ . $wheel- 
>>> >PID);
>>>
>>>         $self->{$PKG}{pids}{$wheel->PID} = $id;
>>>
>>>  so the {pids} are inserted keyed on $wheel->PID. But at line  
>>> 268, the
>>> clean up deletes as follows:
>>>
>>>         delete $self->{$PKG}{wheels}{$id};
>>>         delete $self->{$PKG}{pids}{$id};
>>>
>>>  so it's deleting keyed on wheel->ID, not $wheel->PID.
>>>
>>>  When the PIDs wrap round, the protection in sig_child is broken  
>>> by this:
>>>
>>> sub sig_child {
>>>     my ($kernel, $self, $pid, $rc) = @_[KERNEL, OBJECT, ARG1, ARG2];
>>>
>>>         my $id = $self->{$PKG}{pids}{$pid} || "";
>>>
>>>     # child death signals are issued by the OS and sent to all
>>>     # sessions; we want to handle only our own children
>>>
>>>         return unless $id;
>>>
>>>  Because the {pids} hash had the wrong entry deleted in the  
>>> cleanup, when
>>> the PIDs wrap, an old $id will be found where none should be. So  
>>> sig_child
>>> will not return here, and will cause 'done' to be fired with the old
>>> wheelID, which is plainly wrong.
>>>
>>>  Also, the cleanup around line 268/269 should remove {CLOSED} and
>>> {SIGCHLD} entries for the wheel too. That's less serious because  
>>> it won't
>>> cause a problem until wheel-IDs wrap round, which will take a lot  
>>> longer,
>>> but it's still a leak.
>>>
>>>  Cheers,
>>>  Tony.
>>>
>
>