Bug #103195 for Sub-Name: On \0 in names before ~5.14

Mon Mar 30 04:19:46 2015 ribasushi [...] leporine.io - Ticket created

CC:	sprout [...] cpan.org, pagaltzis [...] gmx.de, rurban [...] cpanel.net
Subject:	On \0 in names before ~5.14

After https://github.com/rurban/sub-name/commit/3dff81c8#commitcomment-10462302 I figured it's worth opening a ticket specifically to track this issue (it was mentioned briefly in RT#69422). This ticket exists to raise 2 questions - Should \0 be allowed by Sub::Name at all, or should the major version be bumped and an explicit exception be thrown - What should Sub::Name do on perls prior to http://perl5.git.perl.org/perl.git/commit/435e8dd0d, where \0 in names is not supported by the interpreter itself The considerations in answering this are the following: - Use cases of anonymouns subroutine naming * The mro *next::method codepaths rely on methods being implmented via properly named coderefs * caller() (and by extension the ecosystem of stack trace generators/analyzers) - Technical validity of \0 in names * Do not work properly at all prior to perl 5.13.7 * Are explicitly supported via tests since http://perl5.git.perl.org/perl.git/blobdiff/9343f4c..9922583:/ext/B/t/b.t or maybe earlier - Logical validity of \0 in names * In perl-land \0 is a character like any other. Allowing it seems sane * In almost all circumstances \0 is not rendered on output, thus making it of questionable (malicious?) utility in caller() and friends I myself am of the opinion: Allow \0 in line with perl5 (that decision has already been made, rehashing it makes no sense), die before 5.13.7. Thoughts?

Tue Mar 31 05:49:19 2015 rurban [...] cpanel.net - Correspondence added

CC:	pagaltzis [...] gmx.de, sprout [...] cpan.org
Subject:	Re: [rt.cpan.org #103195] On \0 in names before ~5.14
Date:	Tue, 31 Mar 2015 11:49:06 +0200
To:	bug-Sub-Name [...] rt.cpan.org
From:	Reini Urban <rurban [...] cpanel.net>

On 03/30/2015 10:19 AM, Peter Rabbitson via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=103195 > > > After https://github.com/rurban/sub-name/commit/3dff81c8#commitcomment-10462302 I figured it's worth opening a ticket specifically to track this issue (it was mentioned briefly in RT#69422). > > This ticket exists to raise 2 questions > - Should \0 be allowed by Sub::Name at all, or should the major version be bumped and an explicit exception be thrown

Of course \0 needs to be allowed. upstream went with this horrible decision even if most modules were not ready. But we have to follow. Show quoted text

> - What should Sub::Name do on perls prior to http://perl5.git.perl.org/perl.git/commit/435e8dd0d, where \0 in names is not supported by the interpreter itself

Of course do the same as before: Silently skip the rest. <5.15 GvNAME had no LEN, it was only a char* Show quoted text

> The considerations in answering this are the following: > > - Use cases of anonymouns subroutine naming > * The mro *next::method codepaths rely on methods being implmented via properly named coderefs > * caller() (and by extension the ecosystem of stack trace generators/analyzers) > > - Technical validity of \0 in names > * Do not work properly at all prior to perl 5.13.7 > * Are explicitly supported via tests since http://perl5.git.perl.org/perl.git/blobdiff/9343f4c..9922583:/ext/B/t/b.t or maybe earlier

And mostly my security concerns, which were not understood and ignored. Years after they started fixing a few cases, but I cannot recommend using perl >5.14 on a public server still. Show quoted text

> - Logical validity of \0 in names > * In perl-land \0 is a character like any other. Allowing it seems sane

Not really. All packagenames went directly without any sanitation to the filesystem, and hence loaded wrong files. You forgot about TR39 Confusables also. Characters are definitely not like any other. How many hidden unicode characters are in a typical program? We can never know because we choose to ignore this problem. Allowing everything is definitely not sane, it is a security risc. http://unicode.org/reports/tr39/ Show quoted text

> * In almost all circumstances \0 is not rendered on output, thus making it of questionable (malicious?) utility in caller() and friends

Not only in caller, until 5.20 in almost all helper functions which prints a name. And in most XS extensions which ignore the LEN. Such as this one. Show quoted text

> I myself am of the opinion: Allow \0 in line with perl5 (that decision has already been made, rehashing it makes no sense), die before 5.13.7.

Not die, skip the rest after \0

Tue Mar 31 05:49:20 2015 The RT System itself - Status changed from 'new' to 'open'

Tue Mar 31 05:54:14 2015 ribasushi [...] leporine.io - Correspondence added

CC:	pagaltzis [...] gmx.de, sprout [...] cpan.org
Subject:	Re: [rt.cpan.org #103195] On \0 in names before ~5.14
Date:	Tue, 31 Mar 2015 11:54:01 +0200
To:	bug-Sub-Name [...] rt.cpan.org
From:	Peter Rabbitson <ribasushi [...] cpan.org>

On 03/31/2015 11:49 AM, Reini Urban via RT wrote: Show quoted text

>

>> I myself am of the opinion: Allow \0 in line with perl5 (that decision has already been made, rehashing it makes no sense), die before 5.13.7.

> Not die, skip the rest after \0 >

This implies that a program will behave differently on < 5.14 and do so *silently*. I think this is a crappy position for a library to be in - the job of a library is to present a consistent interface. Fail closed vs fail open kind of thing. I strongly disagree with "just cut everything after \0". At the very least a warning needs to be issued... In fact one can argue for a warning (think "non portable") on *any* $]. This way a dev sitting on an unrealistically modern 5.20 will be alerted early something isn't right. Thoughts?

Tue Mar 31 09:16:46 2015 rurban [...] cpanel.net - Correspondence added

CC:	pagaltzis [...] gmx.de, sprout [...] cpan.org
Subject:	Re: [rt.cpan.org #103195] On \0 in names before ~5.14
Date:	Tue, 31 Mar 2015 15:16:34 +0200
To:	bug-Sub-Name [...] rt.cpan.org
From:	Reini Urban <rurban [...] cpanel.net>

On 03/31/2015 11:54 AM, Peter Rabbitson via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=103195 > > > On 03/31/2015 11:49 AM, Reini Urban via RT wrote:

>>

>>> I myself am of the opinion: Allow \0 in line with perl5 (that decision has already been made, rehashing it makes no sense), die before 5.13.7.

>> Not die, skip the rest after \0 >>

> > This implies that a program will behave differently on < 5.14 and do so > *silently*. I think this is a crappy position for a library to be in - > the job of a library is to present a consistent interface. Fail closed > vs fail open kind of thing.

These are the decision p5p made, it is not on us to change it. I was the only one who voiced his strong opposition then. Show quoted text

> I strongly disagree with "just cut everything after \0". At the very > least a warning needs to be issued... In fact one can argue for a > warning (think "non portable") on *any* $]. This way a dev sitting on an > unrealistically modern 5.20 will be alerted early something isn't right. > > Thoughts?

I also strongly disagree with all of the p5p decisions in this regard. 1. to silently skip after \0 part for old names, and 2. to silently allow binary names and confusable names since 5.16. Regardless we still need to follow the perl5 API. Which is to silently skip everything after \0 with old names, and to silently allow binary names after 5.16. There is no room for opinions.

Tue Mar 31 15:50:32 2015 ether [...] cpan.org - Correspondence added

On 2015-03-31 06:16:46, rurban@cpanel.net wrote: Show quoted text

> > * In perl-land \0 is a character like any other. Allowing it seems sane

> Not really. All packagenames went directly without any sanitation to the filesystem, and hence loaded wrong files.

I've raised the same issue with respect to unicode package names -- because filesystems do encodings differently (and unpredictably), there's no way of knowing what file the package "Acme::ಠ_ಠ" can be found in. On some filesystems it's 'Acme/ಠ_ಠ.pm', on others (like mine, darwin/HFS+) it's 'Acme/Ã Â²Â _Ã Â²Â.pm'. Show quoted text

> Regardless we still need to follow the perl5 API. > Which is to silently skip everything after \0 with old names, > and to silently allow binary names after 5.16. > > There is no room for opinions.

Does it *have* to be silent? there's no reason why we can't add extra warnings, especially if we create a new warnings category for it that can be disabled.

Wed Apr 01 04:29:42 2015 rurban [...] cpanel.net - Correspondence added

CC:	pagaltzis [...] gmx.de, sprout [...] cpan.org
Subject:	Re: [rt.cpan.org #103195] On \0 in names before ~5.14
Date:	Wed, 01 Apr 2015 10:29:29 +0200
To:	bug-Sub-Name [...] rt.cpan.org, ribasushi [...] cpan.org
From:	Reini Urban <rurban [...] cpanel.net>

On 03/31/2015 09:50 PM, Karen Etheridge via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=103195 > > > On 2015-03-31 06:16:46, rurban@cpanel.net wrote: >

>>> * In perl-land \0 is a character like any other. Allowing it seems sane

>> Not really. All packagenames went directly without any sanitation to the filesystem, and hence loaded wrong files.

> > I've raised the same issue with respect to unicode package names -- because filesystems do encodings differently (and unpredictably), there's no way of knowing what file the package "Acme::ಠ_ಠ" can be found in. On some filesystems it's 'Acme/ಠ_ಠ.pm', on others (like mine, darwin/HFS+) it's 'Acme/Ã Â²Â _Ã Â²Â.pm'.

Also a good point. Encoded paths are not portable. perl and the syscall just ignore the encoding. We can only hope that if the utf8 flag is properly set, the syscall can do utf8. But we don't even support multi-byte syscalls (e.g on windows all countries east of germany are essentially left out) and no translation layers (only symbian, vms and cygwin do this in their ports), so this is a far call. Don't we have a cpan xt test for unportable pathnames already? Show quoted text

>> Regardless we still need to follow the perl5 API. >> Which is to silently skip everything after \0 with old names, >> and to silently allow binary names after 5.16. >> >> There is no room for opinions.

> > Does it *have* to be silent? there's no reason why we can't add extra warnings, especially if we create a new warnings category for it that can be disabled.

Personally I'd like to see a warning. yes. Sure. This would be a big change from perl5 behavior then, and needs to be documented as such. I only filed such a bug report for Package::Stash::XS which invented it's own ASCII-only pathname policy without documentation. https://github.com/doy/package-stash-xs/issues/5

Fri Apr 03 14:51:30 2015 ether [...] cpan.org - Correspondence added

On 2015-04-01 01:29:42, rurban@cpanel.net wrote: Show quoted text

> On 03/31/2015 09:50 PM, Karen Etheridge via RT wrote:

> > <URL: https://rt.cpan.org/Ticket/Display.html?id=103195 > > > > > On 2015-03-31 06:16:46, rurban@cpanel.net wrote: > >

> >>> * In perl-land \0 is a character like any other. Allowing it seems > >>> sane

> >> Not really. All packagenames went directly without any sanitation to > >> the filesystem, and hence loaded wrong files.

> > > > I've raised the same issue with respect to unicode package names -- > > because filesystems do encodings differently (and unpredictably), > > there's no way of knowing what file the package "Acme::ಠ_ಠ" can be > > found in. On some filesystems it's 'Acme/ಠ_ಠ.pm', on others (like > > mine, darwin/HFS+) it's 'Acme/Ã Â²Â _Ã Â²Â.pm'.

> > Also a good point. Encoded paths are not portable. perl and the > syscall > just ignore the encoding. We can only hope that if the utf8 flag is > properly set, the syscall can do utf8. But we don't even support > multi-byte syscalls (e.g on windows all countries east of germany are > essentially left out) and no translation layers (only symbian, vms and > cygwin do this in their ports), so this is a far call. > > Don't we have a cpan xt test for unportable pathnames already?

Yes, there's Test::Portability::Files, but that only encourages using ascii in filenames, and doesn't help us figure out what to do if we want unicode in a package name.

Tue Mar 15 14:16:24 2016 ribasushi [...] leporine.io - Correspondence added

RT-Send-CC:

MITHALDU [...] cpan.org

On Mon Mar 30 10:19:46 2015, RIBASUSHI wrote: Show quoted text

> After https://github.com/rurban/sub- > name/commit/3dff81c8#commitcomment-10462302 I figured it's worth > opening a ticket specifically to track this issue (it was mentioned > briefly in RT#69422).

Closing due to lack of discussion of actual issue. Feel free to reopen if a resource with both XS skills and reason becomes available.

Tue Mar 15 14:16:31 2016 ribasushi [...] leporine.io - Status changed from 'open' to 'rejected'

Tue Aug 16 13:50:49 2016 ether [...] cpan.org - Correspondence added

I think this should stay open so we are aware it's still an issue. I'd forgotten about it until it was mentioned today at https://github.com/moose/Moose/pull/127#issuecomment-240081105.

Tue Aug 16 13:50:49 2016 ether [...] cpan.org - Status changed from 'rejected' to 'stalled'