mojolicious / mojo

:sparkles: Mojolicious - Perl real-time web framework
https://mojolicious.org
Artistic License 2.0
2.66k stars 575 forks source link

Commands error when topic variable set to undef in compilation #2175

Open bbrtj opened 1 month ago

bbrtj commented 1 month ago

Steps to reproduce the behavior

Loop at line 69 of Mojolicious::Commands encounters an undefined value if the topic variable $_ is set to undefined in the compilation phase of that command. Example (on a freshly generated app):

package MyApp::CLI::test;

use v5.38;
use parent 'Mojolicious::Command';

use constant description => 'blah blah';

sub run { ... }

$_ = undef;

Note: In the real project case I'm not setting that value explicitly, I'm using Beam::Wire at compilation phase which most likely does that one way or another because the observed behavior is the same.

Expected behavior

While polluting $_ is obviously erroneous, I believe Commands system should be resistant.

Actual behavior

I get these errors:

Use of uninitialized value $_ in substr at /root/perl5/perlbrew/perls/perl-5.38.0/lib/site_perl/5.38.0/Mojolicious/Commands.pm line 69.
substr outside of string at /root/perl5/perlbrew/perls/perl-5.38.0/lib/site_perl/5.38.0/Mojolicious/Commands.pm line 69.
Use of uninitialized value in hash element at /root/perl5/perlbrew/perls/perl-5.38.0/lib/site_perl/5.38.0/Mojolicious/Commands.pm line 69.
Can't call method "new" on an undefined value at /root/perl5/perlbrew/perls/perl-5.38.0/lib/site_perl/5.38.0/Mojolicious/Commands.pm line 69.

Rewriting the Mojolicious::Commands loop to this form fixes the issue for me:

    for my $pkg (find_modules($ns), find_packages($ns)) {
        next unless _command($pkg);
        $all{substr $pkg, length "${ns}::"} //= $pkg->new->description
    }
Grinnz commented 1 month ago

It is very dangerous to modify $_ without localization in code that may be used elsewhere. The most common cause of this is a while-readline loop that does not assign to a lexical variable.

Grinnz commented 1 month ago

A simpler way to "defang" the $_ alias would be: _command(local $_ = $_), but this would need a comment to explain the oddity.

bbrtj commented 1 month ago

Just a thought: since the code inside grep is loading modules via load_class it's possibly executing a lot of perl code and it's hard to control the scope of it. I believe it's safer to restrain from $_ usage in this case.