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

pp_hot.c:4468: HV *S_opmethod_stash(SV *) Assertion Failed. #15791

Closed
p5pRT opened this issue Jan 3, 2017 · 6 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Jan 3, 2017

Migrated from rt.perl.org#130496 (status was 'resolved')

Searchable as RT130496$

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 3, 2017

From @geeknik

Triggered with Perl v5.25.8-132-gc10193e while fuzzing with AFL.

./perl -e '{0>{}->$}}'
perl​: pp_hot.c​:4468​: HV *S_opmethod_stash(SV *)​: Assertion
`PL_valid_types_PVX[((svtype)((_svpvx)->sv_flags & 0xff)) & 0xf]' failed.
Aborted

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 4, 2017

From @hvds

On Tue, 03 Jan 2017 11​:00​:44 -0800, brian.carpenter@​gmail.com wrote​:

Triggered with Perl v5.25.8-132-gc10193e while fuzzing with AFL.

./perl -e '{0>{}->$}}'
perl​: pp_hot.c​:4468​: HV *S_opmethod_stash(SV *)​: Assertion
`PL_valid_types_PVX[((svtype)((_svpvx)->sv_flags & 0xff)) & 0xf]' failed.
Aborted

This simplifies to​:
% ./miniperl -e '{}->$x'
miniperl​: pp_hot.c​:4470​: S_opmethod_stash​: Assertion `PL_valid_types_PVX[((svtype)((_svpvx)->sv_flags & 0xff)) & 0xf]' failed.
Aborted (core dumped)
%

The crash bisects to Dave's recent commit 9a70c74 "remove DOES's usage of SvSCREAM", I think we're missing a guard of some sort.

Hugo

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 4, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 4, 2017

From @arc

When S_opmethod_stash() determines that the LHS is an unblessed reference, it generates an error message containing the method name. However, it was calling SvPVX() on the method name without ensuring that it's SvPOK(). The fix is straightforward, and is now in blead; see below.

That said, I'm wondering whether we should special-case undefined method names, so as to produce better error messages. If we want to do that before 5.26, we've got about two weeks left before the "user-visible changes" freeze.

commit 323dbd0
Author​: Aaron Crane <arc@​cpan.org>
Date​: Wed Jan 4 20​:02​:45 2017 +0000

  RT#130496​: assertion failure for '{}->$x' on undefined $x

Inline Patch
diff --git a/pp_hot.c b/pp_hot.c
index dd2c611e1f..97d46f6511 100644
--- a/pp_hot.c
+++ b/pp_hot.c
@@ -4465,7 +4465,7 @@ S_opmethod_stash(pTHX_ SV* meth)
                     && SvOBJECT(ob))))
     {
        Perl_croak(aTHX_ "Can't call method \"%" SVf "\" on unblessed reference",
-                  SVfARG((SvPVX(meth) == PL_isa_DOES)
+                  SVfARG((SvPOK(meth) && SvPVX(meth) == PL_isa_DOES)
                                         ? newSVpvs_flags("DOES", SVs_TEMP)
                                         : meth));
     }
diff --git a/t/op/method.t b/t/op/method.t
index 8795734ae4..ef181c4ce0 100644
--- a/t/op/method.t
+++ b/t/op/method.t
@@ -13,7 +13,7 @@ BEGIN {
 use strict;
 no warnings 'once';

-plan(tests => 150);
+plan(tests => 151);

 @A::ISA = 'B';
 @B::ISA = 'C';
@@ -704,6 +704,13 @@ SKIP: {
                   "check unknown import() methods don't corrupt the stack");
 }

+# RT#130496: assertion failure when looking for a method of undefined name
+# on an unblessed reference
+fresh_perl_is('eval { {}->$x }; print $@;',
+              "Can't call method \"\" on unblessed reference at - line 1.",
+              {},
+              "no crash with undef method name on unblessed ref");
+
 __END__
 #FF9900
 #F78C08

-- 

Aaron Crane ** http​://aaroncrane.co.uk/

@p5pRT p5pRT closed this Jan 4, 2017
@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 4, 2017

@arc - Status changed from 'open' to 'resolved'

@p5pRT

This comment has been minimized.

Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 7, 2017

From @iabyn

On Wed, Jan 04, 2017 at 12​:20​:51PM -0800, Aaron Crane via RT wrote​:

That said, I'm wondering whether we should special-case undefined method
names, so as to produce better error messages.

Well currently you get​:

  $ perl -e'my $obj = bless []; my $meth; $obj->$meth'
  Can't locate object method "" via package "main" at -e line 1.

  $ perl -we'my $obj = bless []; my $meth; $obj->$meth'
  Use of uninitialized value $meth in method lookup at -e line 1.
  Can't locate object method "" via package "main" at -e line 1.
  $

which seems reasonably clear to me.

--
If life gives you lemons, you'll probably develop a citric acid allergy.

@p5pRT p5pRT added the Severity Low label Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.