Closed p5pRT closed 12 years ago
From: Rob Nagler \nagler@​bivio\.biz To: boulder-pm@pm.org Subject: Re: [Boulder.pm] mod_perl package lexicals becoming corrupt Date: Wed\, 20 Feb 2008 12:49:32 -0700
Found the bug. Here's what demonstrates it:
package T1; use strict; sub s1 {}
package T2; use base 'T1'; use strict;
package main; T2->s1; print("ok\n"); \&T2::s1; print("not reached\n"); T2->s1;
The \&T2:s1 autovivifies something in the package hash\, which is "nasty"\, and actually ends up corrupting the package hash so that lexicals disappear. We had a function called super_for_method\, which did this:
foreach my $a (@{$proto->inheritance_ancestors}) { my($sub) = \&{$a . '::' . $method}; next unless defined(&$sub); return ($sub\, $a); }
The fact that \&{} seems to autovivify an invalid package hash entry is the problem. You can implement the above this way:
foreach my $a (@{$proto->inheritance_ancestors}) { my($sub) = $a . '::' . $method; do { no strict 'refs'; next unless defined(&$sub); }; return (\&$sub\, $a); }
The above code does not corrupt the the package hash. Both pieces of code autovivify the glob for $sub\, but in the former case\, the autovivification has a dangerous side effect of corrupting the entire symbol table (in a subtle way\, of course\, which is why I got confused by the number of lexicals).
Rob
On Thursday 21 February 2008 16:56:28 nagler@bivio.biz wrote:
# New Ticket Created by nagler@bivio.biz # Please include the string: [perl #51072] # in the subject line of all future correspondence about this issue. # \<URL: http://rt.perl.org/rt3/Ticket/Display.html?id=51072 >
This is a bug report for perl from nagler@bivio.biz\, generated with the help of perlbug 1.35 running under perl v5.8.5.
Under 5.8.8 on 64Bit Ubuntu I get:
# perl t.pl ok not reached Undefined subroutine &T2::s1 called at t.pl line 14.
And after commenting out the \&T2::s1; line:
# perl t.pl ok not reached
All the best\,
Tels
-- Signed on Thu Feb 21 19:58:22 2008 with key 0x93B84C15. Get one of my photo posters: http://bloodgate.com/posters PGP key on http://bloodgate.com/tels.asc or per email.
"If Duke Nukem Forever is not out in 2001\, something's very wrong."
-- George Broussard\, 2001 (http://tinyurl.com/6m8nh)
The RT System itself - Status changed from 'new' to 'open'
On Thu\, Feb 21\, 2008 at 07:56:28AM -0800\, nagler @ bivio. biz wrote:
Found the bug. Here's what demonstrates it:
package T1; use strict; sub s1 \{\} package T2; use base 'T1'; use strict; package main; T2\->s1; print\("ok\\n"\); \\&T2​::s1; print\("not reached\\n"\); T2\->s1;
The \&T2:s1 autovivifies something in the package hash\, which is "nasty"\, and actually ends up corrupting the package hash so that lexicals disappear.
The code above behaves in the manner I'd expect. In what way do you consider it a bug; in what way is the package hash corrupted; and what lexicals have disappeared?
-- You're only as old as you look.
Dave Mitchell wrote:
On Thu\, Feb 21\, 2008 at 07:56:28AM -0800\, nagler @ bivio. biz wrote:
Found the bug. Here's what demonstrates it:
package T1; use strict; sub s1 \{\} package T2; use base 'T1'; use strict; package main; T2\->s1; print\("ok\\n"\); \\&T2​::s1; print\("not reached\\n"\); T2\->s1;
The \&T2:s1 autovivifies something in the package hash\, which is "nasty"\, and actually ends up corrupting the package hash so that lexicals disappear.
The code above behaves in the manner I'd expect. In what way do you consider it a bug; in what way is the package hash corrupted; and what lexicals have disappeared?
I believe the bug/surprising feature is the auto-semi-vivification of T2::s1 and also that it only half exists. This makes it a little clearer.
#!/usr/bin/perl -wl
package T1; sub s1 {}
package T2; use base 'T1';
package main;
T2->s1; print "ok";
print defined &T2::s1 ? "Defined" : "Not defined";
\&T2::s1;
print defined &T2::s1 ? "Defined" : "Not defined";
T2->s1; print "ok"; __END__ Useless use of reference constructor in void context at - line 16. ok Not defined Not defined Undefined subroutine &T2::s1 called at - line 20.
If &T2::s1 is not defined then T2->s1 should call T1->s1. Obviously something has happened to confuse method dispatch.
Before the reference:
DB\<1> x *T2::s1{CODE} 0 undef
After:
DB\<2> x *T2::s1{CODE} 0 CODE(0x191215c) -> &CODE(0x191215c) in ???
It's worth noting that the first call to T2->s1 does make an entry in T2's symbol table.
DB\<1> x \%T2:: 0 HASH(0x1850d54) 'BEGIN' => *T2::BEGIN 'ISA' => *T2::ISA 'isa' => *T2::isa 's1' => *T2::s1
This is most likely method caching. It's possible that \&T2::s1 is messing with that caching.
-- 'All anyone gets in a mirror is themselves\,' she said. 'But what you gets in a good gumbo is everything.' -- "Witches Abroad" by Terry Prachett
On Thu\, February 21\, 2008 5:50 pm\, Michael G Schwern wrote:
If &T2::s1 is not defined then T2->s1 should call T1->s1.
No\, it shouldn't. Please\, let's not discuss this again.
Incorporated by reference: http://nntp.perl.org/group/perl.perl5.porters/130090 and the discussion before it and on perlmonks and Linda's bugs #46987 and #47181.
On Thursday 21 February 2008 17:50:49 Michael G Schwern wrote:
I believe the bug/surprising feature is the auto-semi-vivification of T2::s1 and also that it only half exists.
None of this should be surprising.
This makes it a little clearer.
#!/usr/bin/perl -wl
package T1; sub s1 {}
package T2; use base 'T1';
package main;
T2->s1; print "ok";
print defined &T2::s1 ? "Defined" : "Not defined";
\&T2::s1;
*Something* has to exist for you to take a reference to it. That something doesn't have to be defined\, otherwise you couldn't take references to subs in packages you haven't loaded yet.
If &T2::s1 is not defined then T2->s1 should call T1->s1. Obviously something has happened to confuse method dispatch.
No\, that's not right.
Consider sub predeclarations and AUTOLOAD. This is not a bug in Perl.
Code that treats methods as functions will break in odd (but not surprising) ways. The solution is not to write buggy code.
-- c
chromatic wrote:
On Thursday 21 February 2008 17:50:49 Michael G Schwern wrote:
I believe the bug/surprising feature is the auto-semi-vivification of T2::s1 and also that it only half exists.
None of this should be surprising.
That it is surprising shouldn't be surprising. :) I've been programming Perl for over 10 years and it still catches me off guard. We get reports about this issue on a regular basis. Bug or feature\, it's a trap for users.
Rather then repeating that it's not a bug each time it's reported (and in the cockroach theory of bug reporting there's 999 users who hit this that aren't reporting)\, how do we close this trap?
-- 191. Our Humvees cannot be assembled into a giant battle-robot. -- The 213 Things Skippy Is No Longer Allowed To Do In The U.S. Army http://skippyslist.com/list/
On Feb 21\, 2008\, at 8:58 PM\, Michael G Schwern wrote:
Rather then repeating that it's not a bug each time it's reported
(and in the cockroach theory of bug reporting there's 999 users who
hit this that aren't reporting)\, how do we close this trap?
Document that taking a reference to a sub will cause a sub stub to be
created and method lookup will stop at a stub and look for an
AUTOLOAD\, it will not look beyond the stub in the ISA chain for a sub
with a defined sub.
Graham.
Graham Barr wrote:
On Feb 21\, 2008\, at 8:58 PM\, Michael G Schwern wrote:
Rather then repeating that it's not a bug each time it's reported (and in the cockroach theory of bug reporting there's 999 users who hit this that aren't reporting)\, how do we close this trap?
Document that taking a reference to a sub will cause a sub stub to be created and method lookup will stop at a stub and look for an AUTOLOAD\, it will not look beyond the stub in the ISA chain for a sub with a defined sub.
sub stub stop :)
I have doubts that documenting it will really help. There's likely to be quite a bit of distance between the code which takes the reference and the point where the method is called. That the problem comes from a stray subroutine reference will not be obvious\, so the user is unlikely to find the right spot in the documentation.
All they've got is the error and the error and the code where the error occurs. That's all about methods and subroutines\, not references.
Maybe there's some way to tell the difference between an autovivified\, undefined subroutine and a declared subroutine prototype? That way the resulting error and perldiag entry can be more informative.
-- Just call me 'Moron Sugar'. http://www.somethingpositive.net/sp05182002.shtml
On Thursday 21 February 2008 23:55:19 Michael G Schwern wrote:
I have doubts that documenting it will really help. There's likely to be quite a bit of distance between the code which takes the reference and the point where the method is called. That the problem comes from a stray subroutine reference will not be obvious\, so the user is unlikely to find the right spot in the documentation.
Meanwhile\, people who treat functions and methods differently don't have these kinds of problems. At some point\, someone has to say that bad code only gives you the right behavior by accident.
If you want to fix something\, fix the documentation for Perl objects to explain that:
* SUPER:: is broken for anything other than methods declared directly in the invoking package * everything in UNIVERSAL:: is a method * indirect object syntax has several failure cases that almost no one can enumerate correctly * new is not a keyword * you can't reliably tell if an invocant supports a behavior without trying that behavior * the C implementation type of a referent and the behaviors that referent supports at the Perl level are orthogonal\, and querying the C implementation type to discover which Perl-level behaviors the referent supports is difficult to do correctly * treating methods as functions does not work reliably * most of this can't get fixed until Six
-- c
[Off-list]
chromatic wrote:
On Thursday 21 February 2008 23:55:19 Michael G Schwern wrote:
I have doubts that documenting it will really help. There's likely to be quite a bit of distance between the code which takes the reference and the point where the method is called. That the problem comes from a stray subroutine reference will not be obvious\, so the user is unlikely to find the right spot in the documentation.
Meanwhile\, people who treat functions and methods differently don't have these kinds of problems. At some point\, someone has to say that bad code only gives you the right behavior by accident.
I'm not entirely sure how the "functions as methods" thing relates here. You seem to be beating that drum particularly loudly just now. Why?
Maybe that my $ref = Foo->can('method') is often\, but not always\, a better idea than my $ref = \&Foo::method? Lacking context about the user's problem I can't tell. But it's not terribly useful to the lone user for folks on p5p to say "don't treat methods like functions" and call it a day. Alas\, perl does not come with it's own personal p5p auditor. :)
Much of the language suggests they're interchangeable\, so that's what users are going to do. How do we reverse that? This is both a deep language and documentation issue\, and I'm not saying it's simple.
But for the moment\, can we keep this thread on this one simpler issue?
-- Insulting our readers is part of our business model. http://somethingpositive.net/sp07122005.shtml
On Friday 22 February 2008 00:56:55 Michael G Schwern wrote:
[Off-list]
Err...
I'm not entirely sure how the "functions as methods" thing relates here. You seem to be beating that drum particularly loudly just now. Why?
It's the cause of the problem that Rob demonstrated. It's the cause of the problem that Liz demonstrated.
Mabe that my $ref = Foo->can('method') is often\, but not always\, a better idea than my $ref = \&Foo::method?
That's the only reliable way to take a reference to a method\, and even that's a bad idea because method dispatch is always dynamic. If something changes the characteristics of that dispatch between the time you take that reference and the time you call it\, dispatch will end up elsewhere.
Much of the language suggests they're interchangeable\, so that's what users are going to do. How do we reverse that? This is both a deep language and documentation issue\, and I'm not saying it's simple.
Unless the implementation itself somehow makes a distinction between methods and functions (and the backwards compatibility police are observing your apartment from across the street)\, we don't fix it with code. We fix it with documentation\, with education\, and\, if my TPF grant goes through\, rented time on the Google orbital electromagnet to wipe all bad example code out of the collective Internet memory.
An alternative\, I suppose\, is to get Dave Mitchell really stinkin' drunk in a pub where there are power outlets and wifi available under the table so he can pile hack upon hack to allow method dispatch to jump over declared but undefined code slots when doing actual method dispatch\, not checking can()\, unless there's an applicable AUTOLOAD() somewhere that could possibly get caught. Then you use that Google satellite again to erase his memory of committing the patch\, because it's going to be horrible and ugly hacks upon hacks that make that part of the internals even more opaque and unmaintainable\, if it's even possible.
-- c
On 22/02/2008\, Michael G Schwern \schwern@​pobox\.com wrote:
[Off-list]
Well im replying anyway. :-)
chromatic wrote:
On Thursday 21 February 2008 23:55:19 Michael G Schwern wrote:
I have doubts that documenting it will really help. There's likely to be quite a bit of distance between the code which takes the reference and the point where the method is called. That the problem comes from a stray subroutine reference will not be obvious\, so the user is unlikely to find the right spot in the documentation.
Meanwhile\, people who treat functions and methods differently don't have these kinds of problems. At some point\, someone has to say that bad code only gives you the right behavior by accident.
I'm not entirely sure how the "functions as methods" thing relates here. You seem to be beating that drum particularly loudly just now. Why?
Maybe that my $ref = Foo->can('method') is often\, but not always\, a better idea than my $ref = \&Foo::method? Lacking context about the user's problem I can't tell. But it's not terribly useful to the lone user for folks on p5p to say "don't treat methods like functions" and call it a day. Alas\, perl does not come with it's own personal p5p auditor. :)
Much of the language suggests they're interchangeable\, so that's what users are going to do. How do we reverse that? This is both a deep language and documentation issue\, and I'm not saying it's simple.
They *are* interchangable in many ways. But only if you know what you are doing and the perl way. The docs tend to assume that you do know what you are doing and are pretty familiar with the perl way\, and I think most critically in general are untainted by other OO models.
The real problem is that we have to help people "unlearn" things that other languages have told them are critical aspects of OO. Many of the issues that people get stuck on IME can be traced to deep seated misunderstandings that are hard to tackle because they have been implanted as part of the individuals initial introduction to OO\, and in many cases quite simply do not apply to Perl for one reason or another.
The docs we have seem to be aimed at people who are reasonably open minded about what OO is and is for\, which was probably fine when most people who had OO exposure were also experienced programmers. But nowadays we have loads of people who first learned programming where one form of OO or another is standard and they have absorbed the rules there. They then come to Perl and find out that many of these thing that have been drilled into their head dont apply.
Actually thinking about it more im not sure what you can do about the people that will never manage to unlearn their original mental model of OO. Probably they should just not use Perl.
Yves
-- perl -Mre=debug -e "/just|another|perl|hacker/"
On Fri\, Feb 22\, 2008 at 01:16:33AM -0800\, chromatic wrote:
An alternative\, I suppose\, is to get Dave Mitchell really stinkin' drunk in a pub where there are power outlets and wifi available under the table so he can pile hack upon hack
This one gets my vote ;-)
-- Please note that ash-trays are provided for the use of smokers\, whereas the floor is provided for the use of all patrons. -- Bill Royston
On Thu\, Feb 21\, 2008 at 11:55:19PM -0800\, Michael G Schwern wrote:
Maybe there's some way to tell the difference between an autovivified\, undefined subroutine and a declared subroutine prototype? That way the resulting error and perldiag entry can be more informative.
An undef CV which is the result of a prototype declaration will have a prototype (POK)\, whereas a sub that's been autovivified (or explicitly undefined\, as in 'undef &foo')\, won't.
-- The Enterprise successfully ferries an alien VIP from one place to another without serious incident. -- Things That Never Happen in "Star Trek" #7
On Fri\, Feb 22\, 2008 at 03:47:14PM +0000\, Dave Mitchell wrote:
On Thu\, Feb 21\, 2008 at 11:55:19PM -0800\, Michael G Schwern wrote:
Maybe there's some way to tell the difference between an autovivified\, undefined subroutine and a declared subroutine prototype? That way the resulting error and perldiag entry can be more informative.
An undef CV which is the result of a prototype declaration will have a prototype (POK)\, whereas a sub that's been autovivified (or explicitly undefined\, as in 'undef &foo')\, won't.
Given the context\, I suspect Schwern meant stub\, not prototype.
But we could always add a flag to distinguish the "explicitly declared by stub" /"implicitly declared by reference" cases.
Nicholas Clark
# from nagler@bivio.biz # on Thursday 21 February 2008 07:56:
The \&T2:s1 autovivifies something in the package hash\, which is "nasty"\, and actually ends up corrupting the package hash so that lexicals disappear. We had a function called super_for_method\, which did this:
foreach my $a (@{$proto->inheritance_ancestors}) { my($sub) = \&{$a . '::' . $method}; next unless defined(&$sub); return ($sub\, $a); }
As with all hashes\, exists() is usually the gentler check. You do have to check the symbol table and *then* the & slot\, but this won't auto-vivify.
package T1; use strict; sub s1 {warn "s1"}
package T2; use base 'T1'; use strict;
package main; sub look { warn ''\, ('sym '\, exists(${T2::}{s1}) ? ('exists '\, ('sub '\, exists(&{${T2::}{s1}}) ? 'exists ' : 'does not exist')) : 'does not exist')\, " at line "\, (caller)[2]\, "\n"; } look(); T2->s1; warn "ok"; look();
T2->s1; warn "ok";
{ no strict 'refs'; *{'T2::s1'} = sub {warn "hello";}; } look(); T2->s1; warn "ok";
--Eric -- "Everything should be made as simple as possible\, but no simpler." --Albert Einstein
http://scratchcomputing.com
On 22/02/2008\, Eric Wilhelm \scratchcomputing@​gmail\.com wrote:
# from nagler@bivio.biz # on Thursday 21 February 2008 07:56:
The \&T2:s1 autovivifies something in the package hash\, which is "nasty"\, and actually ends up corrupting the package hash so that lexicals disappear. We had a function called super_for_method\, which did this:
foreach my $a (@{$proto->inheritance_ancestors}) { my($sub) = \&{$a . '::' . $method}; next unless defined(&$sub); return ($sub\, $a); }
As with all hashes\, exists() is usually the gentler check. You do have to check the symbol table and *then* the & slot\, but this won't auto-vivify.
The only thing is (and this is what i meant by deep seated misunderstandings) is that at least some people will expect \&{$a.'::'.$method} to return a method even if its not declared in $a but rather inherited from $a's parent classes.
So the exists approach probably doesnt occur to them as if they are expecting it to be a reference to a parents classes method then exists() would seem to be "obviously" incorrect.
And this also illustrates the problem: A perl person would naturally think of correcting the code by using exists and not even realize that doing so actually solves a different problem then the OP is actually trying to solve.
To me its actually unclear what the OP wants here\, although I too would lean towards the exists solution\, but with the other case that Yitzchak mentioned exists() also seemed appropriate when in fact the OP really wanted can().
Yves
-- perl -Mre=debug -e "/just|another|perl|hacker/"
On Tue Feb 26 09:49:28 2008\, demerphq wrote:
On 22/02/2008\, Eric Wilhelm \scratchcomputing@​gmail\.com wrote:
# from nagler@bivio.biz # on Thursday 21 February 2008 07:56:
Reviewing this older ticket tonight\, my impression was that this discussion among quite a few notable Perl hackers generated a lot of heat but not much light.
Is there anything actionable here for which we should keep this RT open? What\, if anything should be patched?
Thank you very much. Jim Keenan
On Fri Mar 30 17:02:05 2012\, jkeenan wrote:
On Tue Feb 26 09:49:28 2008\, demerphq wrote:
On 22/02/2008\, Eric Wilhelm \scratchcomputing@​gmail\.com wrote:
# from nagler@bivio.biz # on Thursday 21 February 2008 07:56:
Reviewing this older ticket tonight\, my impression was that this discussion among quite a few notable Perl hackers generated a lot of heat but not much light.
Is there anything actionable here for which we should keep this RT open? What\, if anything should be patched?
I think it can simply be closed as not-a-bug.
--
Father Chrysostomos
On Fri Mar 30 18:04:29 2012\, sprout wrote:
On Fri Mar 30 17:02:05 2012\, jkeenan wrote:
On Tue Feb 26 09:49:28 2008\, demerphq wrote:
On 22/02/2008\, Eric Wilhelm \scratchcomputing@​gmail\.com wrote:
# from nagler@bivio.biz # on Thursday 21 February 2008 07:56:
Reviewing this older ticket tonight\, my impression was that this discussion among quite a few notable Perl hackers generated a lot of heat but not much light.
Is there anything actionable here for which we should keep this RT open? What\, if anything should be patched?
I think it can simply be closed as not-a-bug.
Let's do it now before anyone is the wiser!
@jkeenan - Status changed from 'open' to 'rejected'
Migrated from rt.perl.org#51072 (status was 'rejected')
Searchable as RT51072$