Bug #124441 for Object-Trampoline: overeager stash diving

Thu Feb 15 13:03:41 2018 zefram [...] fysh.org - Ticket created

Subject:	overeager stash diving
Date:	Thu, 15 Feb 2018 18:03:30 +0000
To:	bug-Object-Trampoline [...] rt.cpan.org
From:	Zefram <zefram [...] fysh.org>

The test t/03-Universal-Override.t in Object-Trampoline-1.42 fails with Perl 5.27.7 and 5.27.8, because of some changed behaviour in the Carp module. The issue is that some new code in Carp refers to $UNIVERSAL::isa::VERSION, and so vivifies $UNIVERSAL::{"isa::"}. The test iterates over %UNIVERSAL::, using each key found as a method name, and isn't prepared for there to be a key that doesn't syntactically behave as a method name. The test is erroneous in using all keys it finds as method names. It should skip ones that aren't syntactically OK. The Carp module is likely to stop vivifying that stash, but the test is erroneous in any case. -zefram

Sat Mar 03 23:33:32 2018 LEMBARK [...] cpan.org - Correspondence added

Show quoted text

> behave as a method name. > > The test is erroneous in using all keys it finds as method names. > It should skip ones that aren't syntactically OK.

Modified the test to skip ref's without CODE: while( my( $name, $ref ) = each %{ $::{ 'UNIVERSAL::' } } ) { *{ $ref }{ CODE } or next; } Pushing it to CPAN now.

Sat Mar 03 23:33:32 2018 The RT System itself - Status changed from 'new' to 'open'

Sun Mar 04 00:01:19 2018 LEMBARK [...] cpan.org - Correspondence added 5 min

Added test for value in %UNIVERSAL:: having a code ref, skipping symbols that cannot be dispatched. Uploaded as v1.50.2, currently indexed on CPAN.

Sun Mar 04 00:01:20 2018 LEMBARK [...] cpan.org - Status changed from 'open' to 'resolved'

Sun Mar 04 00:01:20 2018 LEMBARK [...] cpan.org - Taken

Fri Mar 09 08:14:11 2018 zefram [...] fysh.org - Correspondence added

Subject:	Re: [rt.cpan.org #124441] overeager stash diving
Date:	Fri, 9 Mar 2018 13:13:54 +0000
To:	Steven Lembark via RT <bug-Object-Trampoline [...] rt.cpan.org>
From:	Zefram <zefram [...] fysh.org>

Steven Lembark via RT wrote: Show quoted text

>Modified the test to skip ref's without CODE:

That's a different issue. Skipping based on the CODE slot doesn't in general solve the problems of using a syntactically invalid method name, though it does avoid the specific instance of it that I described. Also, the code you've added introduces a new problem, in its assumption that the thing it finds in the stash is a glob. The structure of a stash is more complicated than that, and there are multiple kinds of non-glob thing that can be found there. Some of those things represent code objects in that context, and they don't coerce to globs, so this code will miss some kinds of actual method. -zefram

Fri Mar 09 12:06:53 2018 LEMBARK [...] cpan.org - Correspondence added

Show quoted text

> general solve the problems of using a syntactically invalid method name

Show quoted text

> the thing it finds in the stash is a glob. The structure of a stash is > more complicated than that, and there are multiple kinds of non-glob > thing that can be found there. Some of those things represent code > objects in that context, and they don't coerce to globs, so this code > will miss some kinds of actual method.

Short of resorting to XS, the only thing I can think of is checking for invalid names via /\W/, eval-ing the glob expansion, checking for the reftype of the value being code itself. while ( my ( $name, $stash_val ) = each %{ $::{ 'UNIVERSAL::' } } ) { # skip stash entries which cannot map to # valid method names. $name =~ /\W/ and next; eval { # code or globs with a CODE slot # are candidates for mapping. # # eval avoids issues de-referencing # non-glob stash entries. 'CODE' eq reftype $stash_val ? 1 : *{ $stash_val }{ CODE } ? 1 : '' } or next; *{ qualify_to_ref $name } = sub { $AUTOLOAD = $name; goto AUTOLOAD }; }

Fri Mar 09 12:29:39 2018 zefram [...] fysh.org - Correspondence added

Subject:	Re: [rt.cpan.org #124441] overeager stash diving
Date:	Fri, 9 Mar 2018 17:29:23 +0000
To:	Steven Lembark via RT <bug-Object-Trampoline [...] rt.cpan.org>
From:	Zefram <zefram [...] fysh.org>

Steven Lembark via RT wrote: Show quoted text

>checking for invalid names via /\W/

You certainly should do something like that, but watch out for Unicode issues in determining what identifier syntax is and in how regexp features such as \W behave. For example, if you accept non-ASCII characters in identifiers then the character "\x{e9}" should presumably be accepted, but by default "\x{e9}" can match \W, in an instance of the Unicode bug. The core eval() is inconsistent in whether "\x{e9}" is a legal identifier character, in its own instance of the Unicode bug. Also, "\x{308}" is consistently not \W, but whether it's legal in an identifier depends on where it appears in the identifier. To avoid stuff like the "isa::" entry in the UNIVERSAL:: stash, it should suffice to skip only things with ASCII non-identifier characters. But have a think about what you want to do with overloading methods, which have names like "(++". Show quoted text

>eval-ing the glob expansion, checking for the reftype of the value >being code itself.

This still misses REF stash values, which represent constant-valued subs. Even if you add handling for that, this is a maintenance problem: it's code that needs to be updated whenever the details of stash structure change. You can, and should, avoid all of that by ignoring the stash value and doing a proper symbolic lookup from the name, something like: foreach(keys %UNIVERSAL::) { next if /\W/; # subject to discussion above next unless defined &{"UNIVERSAL::$_"}; # $_ is now known to be the name of a method ...; } -zefram