[PATCH] END blocks may segfault

--0016364ecce8d50a63048a1b1c6c
Content-Type: text/plain; charset=UTF-8

Hi!

perl: 5.10.1, 5.12.1 (i686-linux, x86_64-linux)
mod_perl: 2.0.4

Basically if you have end blocks that modify/add END blocks things
might go crazy.

A simple test case for this is:
package test:
END { eval "END { }" for 1..10 }

gives:
Not a CODE reference.
END failed--call queue aborted.

if I add another END {}:
package test:
END { eval "END { }" for 1..10 }
END { }

I get:
Undefined subroutine &main:: called.
END failed--call queue aborted.

This was found as I was getting strange segfaults when trying to use
NYTProf. Nicholas Clark was able to pinpoint the problem.

See:
http://groups.google.com/group/develnytprof-dev/msg/4ff26e6b 3d95861f
thread: http://groups.google.com/group/develnytprof-dev/browse_threa d/thread/20d2ea800e8936f

Ive included some relevant bits here (again from Nicholas):

> Of course, I looked at the core perl source to see how END blocks were run,
> and the behaviour was safe with perl. perl unshifts each CV from the list
> before it calls it. I should have remembered that inevitably mod_perl is
> "special".
>
> On reflection, I think that this *is* a bug in mod_perl, as they're going
> to get tripped up in similar fashion if any code run as part of an END block
> contains a string eval containing an END block.

Now the simple fix would be to just make modperl shift things off as
well (see attached patch to make it use Perl_call_list). Course if
they use ModPerl::Global::special_list_(call|clear) like
t/modperl/endav.t things break. As when we go to run END blocks or
call special_list_call() they will have been shifted off. This might
be acceptable behavior for END blocks, but I can see not wanting to
break other things that use modperl_perl_call_use. Maybe, maybe not.
Thoughts?

Another option would be to copy/local() the array in
modpler_perl_call_list. Of course that still wont get it "right"
because we wont run any newly defined subs... But at least we wont
segfault/crap out.

Thoughts?

--0016364ecce8d50a63048a1b1c6c
Content-Type: text/x-patch; charset=US-ASCII; name="modperl2_use_perl_call_list.patch"
Content-Disposition: attachment;
filename="modperl2_use_perl_call_list.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_gazka9go0

LS0tIG1vZHBlcmxfdXRpbC5jLm9yaWcJMjAxMC0wNi0yOCAxMDo1ODo1My41 MzU2ODYyNTQgLTA2
MDAKKysrIG1vZHBlcmxfdXRpbC5jCTIwMTAtMDYtMjggMTE6MDE6MjUuMDU5 MTEyMDU5IC0wNjAw
CkBAIC00NjcsMzAgKzQ2NywxMiBAQAogCiB2b2lkIG1vZHBlcmxfcGVybF9j YWxsX2xpc3QocFRI
WF8gQVYgKnN1YnMsIGNvbnN0IGNoYXIgKm5hbWUpCiB7Ci0gICAgSTMyIGks IG9sZHNjb3BlID0g
UExfc2NvcGVzdGFja19peDsKLSAgICBTViAqKmFyeSA9IEF2QVJSQVkoc3Vi cyk7Ci0KICAgICBN
UF9UUkFDRV9nKE1QX0ZVTkMsICJwaWQgJWx1IiBNUF9UUkFDRWZfVElEIE1Q X1RSQUNFZl9QRVJM
SUQKICAgICAgICAgICAgICAgICIgcnVubmluZyAlZCAlcyBzdWJzIiwKICAg ICAgICAgICAgICAg
ICh1bnNpZ25lZCBsb25nKWdldHBpZCgpLCBNUF9UUkFDRXZfVElEXyBNUF9U UkFDRXZfUEVSTElE
XwogICAgICAgICAgICAgICAgQXZGSUxMcChzdWJzKSsxLCBuYW1lKTsKIAot ICAgIGZvciAoaT0w
OyBpPD1BdkZJTExwKHN1YnMpOyBpKyspIHsKLSAgICAgICAgQ1YgKmN2ID0g KENWKilhcnlbaV07
Ci0gICAgICAgIFNWICphdHN2ID0gRVJSU1Y7Ci0KLSAgICAgICAgUFVTSE1B UksoUExfc3RhY2tf
c3ApOwotICAgICAgICBjYWxsX3N2KChTViopY3YsIEdfRVZBTHxHX0RJU0NB UkQpOwotCi0gICAg
ICAgIGlmIChTdkNVUihhdHN2KSkgewotICAgICAgICAgICAgUGVybF9zdl9j YXRwdmYoYVRIWF8g
YXRzdiwgIiVzIGZhaWxlZC0tY2FsbCBxdWV1ZSBhYm9ydGVkIiwKLSAgICAg ICAgICAgICAgICAg
ICAgICAgICAgIG5hbWUpOwotICAgICAgICAgICAgd2hpbGUgKFBMX3Njb3Bl c3RhY2tfaXggPiBv
bGRzY29wZSkgewotICAgICAgICAgICAgICAgIExFQVZFOwotICAgICAgICAg ICAgfQotICAgICAg
ICAgICAgUGVybF9jcm9hayhhVEhYXyAiJXMiLCBTdlBWWChhdHN2KSk7Ci0g ICAgICAgIH0KLSAg
ICB9CisgICAgUGVybF9jYWxsX2xpc3QoYVRIWF8gUExfc2NvcGVzdGFja19p eCwgc3Vicyk7CiB9
CiAKIHZvaWQgbW9kcGVybF9wZXJsX2V4aXQocFRIWF8gaW50IHN0YXR1cykK
--0016364ecce8d50a63048a1b1c6c--

Re: [PATCH] END blocks may segfault

On Mon, Jun 28, 2010 at 11:15 AM, Alex Hunsaker wrote:
> Hi!
>
> perl: 5.10.1, 5.12.1 (i686-linux, x86_64-linux)
> mod_perl: 2.0.4
>
> Basically if you have end blocks that modify/add END blocks things
> might go crazy.

This sounds like it is similar to something Tim B. brought up a month
or so ago dealing with the new NYTProf release, but I wasn't able to
find the thread. Is this issue related?

> A simple test case for this is:
> package test:
> END { eval "END { }" for 1..10 }
>
> gives:
> Not a CODE reference.
> END failed--call queue aborted.
>
> if I add another END {}:
> package test:
> END { eval "END { }" for 1..10 }
> END { }
>
> I get:
> Undefined subroutine &main:: called.
> END failed--call queue aborted.
>
> This was found as I was getting strange segfaults when trying to use
> NYTProf. =A0Nicholas Clark was able to pinpoint the problem.
>
> See:
> http://groups.google.com/group/develnytprof-dev/msg/4ff26e6b 3d95861f
> thread: http://groups.google.com/group/develnytprof-dev/browse_threa d/thr=
ead/20d2ea800e8936f
>
> Ive included some relevant bits here (again from Nicholas):
>
>> Of course, I looked at the core perl source to see how END blocks were r=
un,
>> and the behaviour was safe with perl. perl unshifts each CV from the lis=
t
>> before it calls it. I should have remembered that inevitably mod_perl is
>> "special".
>>
>> On reflection, I think that this *is* a bug in mod_perl, as they're goin=
g
>> to get tripped up in similar fashion if any code run as part of an END b=
lock
>> contains a string eval containing an END block.

That doesn't seem like a widespread use case, but I can see how it
could occur unexpectedly and generate a fair amount of pain. But from
the times that I have used string eval's, it was always accompanied by
some pain.

> Now the simple fix would be to just make modperl shift things off as
> well (see attached patch to make it use Perl_call_list). =A0Course if
> they use ModPerl::Global::special_list_(call|clear) like
> t/modperl/endav.t things break. =A0As when we go to run END blocks or
> call special_list_call() =A0they will have been shifted off. =A0This migh=
t
> be acceptable behavior for END blocks, but I can see not wanting to
> break other things that use modperl_perl_call_use. =A0Maybe, maybe not.
> Thoughts?

ModPerl::RegistryCooker appears to use the special_list calls.

http://perl.apache.org/docs/2.0/api/ModPerl/Global.html

So the fix could introduce some breakage into deployments which use MP::RC

> Another option would be to copy/local() the array in
> modpler_perl_call_list. =A0Of course that still wont get it "right"
> because we wont run any newly defined subs... But at least we wont
> segfault/crap out.
>
> Thoughts?

I think getting rid of the segfault is a good thing. But if the main
problem is issues with NYTProf, then it seems like this change won't
solve the core problem of autogenerated null end blocks faulting. I'm
don't fully understand the scope of these issues yet, so my answers
(and questions) may not be completely helpful.

Re: [PATCH] END blocks may segfault

On Tue, Jun 29, 2010 at 09:50:00PM -0700, Fred Moyer wrote:
> On Mon, Jun 28, 2010 at 11:15 AM, Alex Hunsaker wro=
te:
>=20
> > Another option would be to copy/local() the array in
> > modpler_perl_call_list. =A0Of course that still wont get it "right"
> > because we wont run any newly defined subs... But at least we wont
> > segfault/crap out.
> >
> > Thoughts?
>=20
> I think getting rid of the segfault is a good thing. But if the main
> problem is issues with NYTProf, then it seems like this change won't
> solve the core problem of autogenerated null end blocks faulting. I'm
> don't fully understand the scope of these issues yet, so my answers
> (and questions) may not be completely helpful.

I suggest the code shift items off the main array, like perl does,
but also push those items onto a new temp array.
When there are no more items in the main array it would copy
the items back again (av_make() is handy for that).

Tim.

Re: [PATCH] END blocks may segfault

On Wed, Jun 30, 2010 at 02:49, Tim Bunce wrote:
> On Tue, Jun 29, 2010 at 09:50:00PM -0700, Fred Moyer wrote:
>> I think getting rid of the segfault is a good thing.  But if the ma=
in
>> problem is issues with NYTProf, then it seems like this change won't
>> solve the core problem of autogenerated null end blocks faulting.  =
I'm
>> don't fully understand the scope of these issues yet, so my answers
>> (and questions) may not be completely helpful.

Well, the point of the test case is it does not use NYTProf. Sure It
was found while playing with NYTProf. :)

> I suggest the code shift items off the main array, like perl does,
> but also push those items onto a new temp array.
> When there are no more items in the main array it would copy
> the items back again (av_make() is handy for that).

Wouldn't the list keep getting longer each time? For example take
your test case:
END { eval "END { }" for 1..10 }

The first time we run them we should shift of 11 items. The next time
21, etc. Am I missing something? Perhaps some cool trick? Thats why
I was thinking local() or equiv would make the most sense.

Hrm?

Re: [PATCH] END blocks may segfault

On Wed, Jun 30, 2010 at 09:24:42AM -0600, Alex Hunsaker wrote:
> On Wed, Jun 30, 2010 at 02:49, Tim Bunce wrote:
> > On Tue, Jun 29, 2010 at 09:50:00PM -0700, Fred Moyer wrote:
> >> I think getting rid of the segfault is a good thing. =A0But if the m=
ain
> >> problem is issues with NYTProf, then it seems like this change won't
> >> solve the core problem of autogenerated null end blocks faulting. =A0=
I'm
> >> don't fully understand the scope of these issues yet, so my answers
> >> (and questions) may not be completely helpful.
>=20
> Well, the point of the test case is it does not use NYTProf. Sure It
> was found while playing with NYTProf. :)
>=20
> > I suggest the code shift items off the main array, like perl does,
> > but also push those items onto a new temp array.
> > When there are no more items in the main array it would copy
> > the items back again (av_make() is handy for that).
>=20
> Wouldn't the list keep getting longer each time? For example take
> your test case:
> END { eval "END { }" for 1..10 }
>=20
> The first time we run them we should shift of 11 items. The next time
> 21, etc. Am I missing something? Perhaps some cool trick? Thats why
> I was thinking local() or equiv would make the most sense.

I presume mod_perl doesn't shift the array because it wants the array to
persist to be used again later. If not then it should shift.

Otherwise running 11 the first and 21 the second is the right behavior
for this (very unusual) test case.

Tim.

p.s. I'm not familar enough with mod_perl not know the all issues
surrounding how it uses PL_endav. So if this thread continues I'd be
grateful if someone could give some background/context. Thanks.

Re: [PATCH] END blocks may segfault

On Wed, Jun 30, 2010 at 16:37, Tim Bunce wrote:
> On Wed, Jun 30, 2010 at 09:24:42AM -0600, Alex Hunsaker wrote:
>> On Wed, Jun 30, 2010 at 02:49, Tim Bunce wrote:
>> > I suggest the code shift items off the main array, like perl does,
>> > but also push those items onto a new temp array.

> I presume mod_perl doesn't shift the array because it wants the array to
> persist to be used again later.

Yeah... Basically when running under ModPerl::Registry it tries to
treat your script exactly like it is running under cgi. So when the
script finish it runs end blocks. When the next request comes in you
still want those end blocks to run.

From the ModPerl::Registry documentation:
| END blocks encountered during compilation of a script, are called
| after the script has completed its run, including subsequent
| invocations when the script is cached in memory. This is
| assuming that the script itself doesn't define a package on its
| own. If the script defines its own package, the END blocks in the
| scope of that package will be executed at the end of the interpretor's life.

> Otherwise running 11 the first and 21 the second is the right behavior
> for this (very unusual) test case.

Yeah... I think you are right, I was completely over thinking things I think.

Re: [PATCH] END blocks may segfault

--00c09f972932ff7dfb048a6f2e8e
Content-Type: text/plain; charset=UTF-8

>>> On Wed, Jun 30, 2010 at 02:49, Tim Bunce wrote:
>>> > I suggest the code shift items off the main array, like perl does,
>>> > but also push those items onto a new temp array.

Find attached a patch that does the above (perhaps naively), passes a
make test and fixes the reported problems for me.

--00c09f972932ff7dfb048a6f2e8e
Content-Type: text/x-patch; charset=US-ASCII; name="modperl_util_call_list_shift.patch"
Content-Disposition: attachment;
filename="modperl_util_call_list_shift.patch"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_gb5lq1gg0

ZGlmZiAtLWdpdCBhL3NyYy9tb2R1bGVzL3BlcmwvbW9kcGVybF91dGlsLmMg Yi9zcmMvbW9kdWxl
cy9wZXJsL21vZHBlcmxfdXRpbC5jCmluZGV4IGNlOTU0NjYuLjdhNzRiZTAg MTAwNjQ0Ci0tLSBh
L3NyYy9tb2R1bGVzL3BlcmwvbW9kcGVybF91dGlsLmMKKysrIGIvc3JjL21v ZHVsZXMvcGVybC9t
b2RwZXJsX3V0aWwuYwpAQCAtNDY3LDE5ICs0NjcsMjIgQEAgdm9pZCBtb2Rw ZXJsX3BlcmxfZG9f
c3ByaW50ZihwVEhYXyBTViAqc3YsIEkzMiBsZW4sIFNWICoqc2FyZykKIAog dm9pZCBtb2RwZXJs
X3BlcmxfY2FsbF9saXN0KHBUSFhfIEFWICpzdWJzLCBjb25zdCBjaGFyICpu YW1lKQogewotICAg
IEkzMiBpLCBvbGRzY29wZSA9IFBMX3Njb3Blc3RhY2tfaXg7Ci0gICAgU1Yg KiphcnkgPSBBdkFS
UkFZKHN1YnMpOworICAgIEkzMiBvbGRzY29wZSA9IFBMX3Njb3Blc3RhY2tf aXg7CisgICAgQVYg
KnRtcGF2ID0gbmV3QVYoKTsKKworICAgIGF2X2V4dGVuZCh0bXBhdiwgQXZG SUxMcChzdWJzKSk7
CiAKICAgICBNUF9UUkFDRV9nKE1QX0ZVTkMsICJwaWQgJWx1IiBNUF9UUkFD RWZfVElEIE1QX1RS
QUNFZl9QRVJMSUQKICAgICAgICAgICAgICAgICIgcnVubmluZyAlZCAlcyBz dWJzIiwKICAgICAg
ICAgICAgICAgICh1bnNpZ25lZCBsb25nKWdldHBpZCgpLCBNUF9UUkFDRXZf VElEXyBNUF9UUkFD
RXZfUEVSTElEXwogICAgICAgICAgICAgICAgQXZGSUxMcChzdWJzKSsxLCBu YW1lKTsKIAotICAg
IGZvciAoaT0wOyBpPD1BdkZJTExwKHN1YnMpOyBpKyspIHsKLSAgICAgICAg Q1YgKmN2ID0gKENW
KilhcnlbaV07CisgICAgd2hpbGUgKGF2X2xlbihzdWJzKSA+PSAwKSB7Cisg ICAgICAgIENWICpj
diA9IChDViopYXZfc2hpZnQoc3Vicyk7CiAgICAgICAgIFNWICphdHN2ID0g RVJSU1Y7CiAKICAg
ICAgICAgUFVTSE1BUksoUExfc3RhY2tfc3ApOworICAgICAgICBhdl9wdXNo KHRtcGF2LCAoU1Yq
KWN2KTsKICAgICAgICAgY2FsbF9zdigoU1YqKWN2LCBHX0VWQUx8R19ESVND QVJEKTsKIAogICAg
ICAgICBpZiAoU3ZDVVIoYXRzdikpIHsKQEAgLTQ5MSw2ICs0OTQsMTEgQEAg dm9pZCBtb2RwZXJs
X3BlcmxfY2FsbF9saXN0KHBUSFhfIEFWICpzdWJzLCBjb25zdCBjaGFyICpu YW1lKQogICAgICAg
ICAgICAgUGVybF9jcm9hayhhVEhYXyAiJXMiLCBTdlBWWChhdHN2KSk7CiAg ICAgICAgIH0KICAg
ICB9CisKKyAgICB3aGlsZSAoYXZfbGVuKHRtcGF2KSA+PSAwKSB7CisgICAg ICAgIGF2X3B1c2go
c3VicywgYXZfc2hpZnQodG1wYXYpKTsKKyAgICB9CisgICAgc3ZfZnJlZSgo U1YqKXRtcGF2KTsK
IH0KIAogdm9pZCBtb2RwZXJsX3BlcmxfZXhpdChwVEhYXyBpbnQgc3RhdHVz KQo=
--00c09f972932ff7dfb048a6f2e8e--