mojo icon indicating copy to clipboard operation
mojo copied to clipboard

Commands error when topic variable set to undef in compilation

Open bbrtj opened this issue 1 year ago • 3 comments

  • Mojolicious version: 9.30
  • Perl version: 5.38
  • Operating system: FreeBSD 14.0

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
	}

bbrtj avatar May 07 '24 19:05 bbrtj

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 avatar May 07 '24 19:05 Grinnz

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

Grinnz avatar May 07 '24 19:05 Grinnz

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.

bbrtj avatar May 07 '24 19:05 bbrtj