Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bbrtj opened this issue May 7, 2024 · 3 comments · May be fixed by #2181
Open

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

bbrtj opened this issue May 7, 2024 · 3 comments · May be fixed by #2181

Comments

@bbrtj
Copy link

bbrtj commented May 7, 2024

  • 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
	}
@Grinnz
Copy link
Contributor

Grinnz commented May 7, 2024

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
Copy link
Contributor

Grinnz commented May 7, 2024

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

@bbrtj
Copy link
Author

bbrtj commented May 7, 2024

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 added a commit to bbrtj/mojo that referenced this issue May 17, 2024
bbrtj added a commit to bbrtj/mojo that referenced this issue May 17, 2024
bbrtj added a commit to bbrtj/mojo that referenced this issue May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants