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

Perl_sv_2pv_flags: Assertion `((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed #14668

Closed
p5pRT opened this issue Apr 23, 2015 · 30 comments
Labels

Comments

@p5pRT
Copy link
Collaborator

@p5pRT p5pRT commented Apr 23, 2015

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

Searchable as RT124368$

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2015

From @geeknik

Built v5.21.12 (v5.21.11-10-ga8f582b) using the following command line​:

./Configure -des -Dusedevel -DDEBUGGING -Dcc=afl-gcc -Doptimize=-O2\ -g && AFL_HARDEN=1 make -j6 test-prep

Bug found with AFL (http​://lcamtuf.coredump.cx/afl)

GDB output​:
perl​: sv.c​:2964​: Perl_sv_2pv_flags​: Assertion `((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.

Program received signal SIGABRT, Aborted.
[----------------------------------registers-----------------------------------]
RAX​: 0x0
RBX​: 0x7fffffffe624 --> 0x736574006c726570 ('perl')
RCX​: 0xffffffffffffffff
RDX​: 0x6
RSI​: 0x126d
RDI​: 0x126d
RBP​: 0x7ffff6ea9c67 --> 0x257325732500203a ('​: ')
RSP​: 0x7fffffffde68 --> 0x7ffff6d933e0 (<*__GI_abort+384>​: mov rdx,QWORD PTR fs​:0x10)
RIP​: 0x7ffff6d90165 (<*__GI_raise+53>​: cmp rax,0xfffffffffffff000)
R8 : 0x7ffff7fdc700 (0x00007ffff7fdc700)
R9 : 0x5653203d21202929 (')) != SV')
R10​: 0x8
R11​: 0x202
R12​: 0xf3b378 ("((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM")
R13​: 0xf4fab0 ("Perl_sv_2pv_flags")
R14​: 0x7ffff6ea9c67 --> 0x257325732500203a ('​: ')
R15​: 0xb94
EFLAGS​: 0x202 (carry parity adjust zero sign trap INTERRUPT direction overflow)
[-------------------------------------code-------------------------------------]
  0x7ffff6d9015b <*__GI_raise+43>​: movsxd rdi,eax
  0x7ffff6d9015e <*__GI_raise+46>​: mov eax,0xea
  0x7ffff6d90163 <*__GI_raise+51>​: syscall
=> 0x7ffff6d90165 <*__GI_raise+53>​: cmp rax,0xfffffffffffff000
  0x7ffff6d9016b <*__GI_raise+59>​: ja 0x7ffff6d90182 <*__GI_raise+82>
  0x7ffff6d9016d <*__GI_raise+61>​: repz ret
  0x7ffff6d9016f <*__GI_raise+63>​: nop
  0x7ffff6d90170 <*__GI_raise+64>​: test eax,eax
[------------------------------------stack-------------------------------------]
0000| 0x7fffffffde68 --> 0x7ffff6d933e0 (<*__GI_abort+384>​: mov rdx,QWORD PTR fs​:0x10)
0008| 0x7fffffffde70 --> 0xf3b378 ("((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM")
0016| 0x7fffffffde78 --> 0x7ffff6eabc21 --> 0x706c6568007325 ('%s')
0024| 0x7fffffffde80 --> 0x7fffffffdea0 --> 0x3000000018
0032| 0x7fffffffde88 --> 0xb94
0040| 0x7fffffffde90 --> 0x7fffffffdf90 --> 0x7fffffffe624 --> 0x736574006c726570 ('perl')
0048| 0x7fffffffde98 --> 0x7ffff6dc41b6 (<__fxprintf+310>​: lea rsp,[rbp-0x20])
0056| 0x7fffffffdea0 --> 0x3000000018
[------------------------------------------------------------------------------]
Legend​: code, data, rodata, value
Stopped reason​: SIGABRT
0x00007ffff6d90165 in *__GI_raise (sig=<optimized out>)
  at ../nptl/sysdeps/unix/sysv/linux/raise.c​:64
64 ../nptl/sysdeps/unix/sysv/linux/raise.c​: No such file or directory.

System Info​: Debian 7, Kernel 3.2.65-1+deb7u2 x86_64, GCC 4.9.2, libc 2.13-38+deb7u8

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2015

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2015

From @geeknik

I minimized the attached test case with afl-tmin from 469 bytes to 50 bytes, however, the minimized test case didn't cause the assertion failure and caused GDB and Valgrind to hang.

Reduced test case​:
map{s[][]o=~@​0,$0[0]>map{0?{s()()}​:0}0}<>__END__
0

Hexdump​:
0000000 616d 7b70 5b73 5b5d 6f5d 7e3d 3040 242c
0000010 5b30 5d30 6d3e 7061 307b 7b3f 2873 2829
0000020 7d29 303a 307d 3c7d 5f3e 455f 444e 5f5f
0000030 300a
0000032

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 23, 2015

From [Unknown Contact. See original ticket]

I minimized the attached test case with afl-tmin from 469 bytes to 50 bytes, however, the minimized test case didn't cause the assertion failure and caused GDB and Valgrind to hang.

Reduced test case​:
map{s[][]o=~@​0,$0[0]>map{0?{s()()}​:0}0}<>__END__
0

Hexdump​:
0000000 616d 7b70 5b73 5b5d 6f5d 7e3d 3040 242c
0000010 5b30 5d30 6d3e 7061 307b 7b3f 2873 2829
0000020 7d29 303a 307d 3c7d 5f3e 455f 444e 5f5f
0000030 300a
0000032

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 29, 2015

From @iabyn

On Thu, Apr 23, 2015 at 01​:57​:08PM -0700, Brian Carpenter via RT wrote​:

I minimized the attached test case with afl-tmin from 469 bytes to 50
bytes, however, the minimized test case didn't cause the assertion
failure and caused GDB and Valgrind to hang.

I can't reproduce the failure with the original code (no assertion
failure; valgrind and ASan ok).

Reduced test case​:
map{s[][]o=~@​0,$0[0]>map{0?{s()()}​:0}0}<>__END__
0

That hangs because the <> is reading from stdin.

--
All wight. I will give you one more chance. This time, I want to hear
no Wubens. No Weginalds. No Wudolf the wed-nosed weindeers.
  -- Life of Brian

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 29, 2015

From @hvds

On Wed Apr 29 04​:14​:52 2015, davem wrote​:

I can't reproduce the failure with the original code (no assertion
failure; valgrind and ASan ok).

I can, with a plain build of blead. Most of the original code is red herrings, I was able to reduce it to this​:

% ./miniperl -e '@​a = qw{x y}; for (@​a) { s{.}{}go =~ @​b }'
miniperl​: sv.c​:2964​: Perl_sv_2pv_flags​: Assertion `((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.
Aborted (core dumped)
%

Are you able to reproduce it that way? If not I may be able to make some time to dig more over the weekend.

Hugo

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 29, 2015

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

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Apr 30, 2015

From @tonycoz

On Wed Apr 29 12​:07​:25 2015, hv wrote​:

On Wed Apr 29 04​:14​:52 2015, davem wrote​:

I can't reproduce the failure with the original code (no assertion
failure; valgrind and ASan ok).

I can, with a plain build of blead. Most of the original code is red
herrings, I was able to reduce it to this​:

% ./miniperl -e '@​a = qw{x y}; for (@​a) { s{.}{}go =~ @​b }'
miniperl​: sv.c​:2964​: Perl_sv_2pv_flags​: Assertion `((svtype)((sv)-

sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) !=
SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.
Aborted (core dumped)
%

Are you able to reproduce it that way? If not I may be able to make
some time to dig more over the weekend.

The problem seems to be this code in pp_regcomp​:

  /* can't change the optree at runtime either */
  /* PMf_KEEP is handled differently under threads to avoid these problems */
  if (!RX_PRELEN(PM_GETRE(pm)) && PL_curpm)
  pm = PL_curpm;
  if (pm->op_pmflags & PMf_KEEP) {
  pm->op_private &= ~OPpRUNTIME; /* no point compiling again */
  cLOGOP->op_first->op_next = PL_op->op_next;
  }

before this starts pm point at the match op and PL_curpm points at the subst op.

RX_PRELEN() is zero for rx, so pm is updated, and since the subst has the /o flag (PMf_KEEP) the op tree is rewritten to skip the regcomp for the second time around the loop, leaving the raw AV on the stack as the regexp for pp_match.

Hugo's example doesn't need the /g flag​:

$ ./miniperl -e '@​a = qw{x y}; for (@​a) { s{.}{}o =~ @​b }'
miniperl​: sv.c​:2964​: Perl_sv_2pv_flags​: Assertion `((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.
Aborted

Tested on a non-threaded perl.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 8, 2015

From @iabyn

On Wed, Apr 29, 2015 at 10​:22​:00PM -0700, Tony Cook via RT wrote​:

On Wed Apr 29 12​:07​:25 2015, hv wrote​:

On Wed Apr 29 04​:14​:52 2015, davem wrote​:

I can't reproduce the failure with the original code (no assertion
failure; valgrind and ASan ok).

I can, with a plain build of blead. Most of the original code is red
herrings, I was able to reduce it to this​:

% ./miniperl -e '@​a = qw{x y}; for (@​a) { s{.}{}go =~ @​b }'
miniperl​: sv.c​:2964​: Perl_sv_2pv_flags​: Assertion `((svtype)((sv)-

sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) !=
SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.
Aborted (core dumped)
%

Are you able to reproduce it that way? If not I may be able to make
some time to dig more over the weekend.

The problem seems to be this code in pp_regcomp​:

/\* can't change the optree at runtime either \*/
/\* PMf\_KEEP is handled differently under threads to avoid these problems \*/
if \(\!RX\_PRELEN\(PM\_GETRE\(pm\)\) && PL\_curpm\)
pm = PL\_curpm;
if \(pm\->op\_pmflags & PMf\_KEEP\) \{
pm\->op\_private &= ~OPpRUNTIME;    /\* no point compiling again \*/
cLOGOP\->op\_first\->op\_next = PL\_op\->op\_next;
\}

before this starts pm point at the match op and PL_curpm points at the subst op.

RX_PRELEN() is zero for rx, so pm is updated, and since the subst has
the /o flag (PMf_KEEP) the op tree is rewritten to skip the regcomp for
the second time around the loop, leaving the raw AV on the stack as the
regexp for pp_match.

This bug has in fact been present since 5.0 or before; it's just that
a) a change in 5.18.0 made "$_ =~ @​a", where @​a is empty, match "@​a" (i.e.
"") rather than the string "0"; and b) an assertion added in 5.19.x made
getting the string value of an AV an error. So I don't think that it needs
to be a 5.22 blocker any more.

The main issue is the mis-implementation and interaction between two
regexp "features"; first that the 'o' flag in /$expr/o means that the
pattern is only compiled once, and second that in /$expr/, if $expr
evaluates to the empty string, then the last successful regex is used
instead. It is documented in perlop that​:

  In this case, only the C<g> and C<c> flags on the empty pattern are
  honored; the other flags are taken from the original pattern

So in something like the following​:

  /./o;
  $_ =~ @​b;

if @​b is empty, then (since 5.18.0), it evaluates to the empty string,
so the null pattern rule kicks in, and the /./ pattern is used instead.
Then the "other flags are taken from the original pattern" rule kicks
in, so the //o flag from the earlier successful match is honoured,
so the optree of the current match ($_ =~ @​b), i.e. the one *without*
the /o flag, is manipulated to make it skip future compiles. Since that
optree wasn't compiled with //o present, it doesn't have the extra
regcmaybe op in the tree, which the code blindly assumes is the first
child of the regcomp op, and so happily corrupts that tree.

This is fixable, but first we need to agree on what the semantics should
be, in particular, in the following​:

  for (...) {
  /abc/o;
  /$expr/;
  }

on executing /$expr/ if $expr evaluates to the null string, should it
honour the //o of the previous successful match, and so skip evaluating
$expr on subsequent iterations? This would be hard to implement.

Conversely, should /$expr/o honour the //o flag even if $expr is null?

My opinion is that, like c and g, the o flag should be honoured on the
match op it appears on, not on the last successful match op, and we fix
the docs (as well as the code).

So I propose that​:

  /abc/o; $foo =~ /$null/

matches $foo against "abc" each time round the loop (it currently crashes
on the second iteration); and that​:

  /abc/; $foo =~ /$null/o

always only evaluates $null once; if that happens to be the empty string,
then subsequent iterations will see the empty string each time and so each
time use the last successful match, which may be /abc/ (it currently
ignores the //o on the first iteration if $null is empty).

Note that nothing in the test suite currently tests these cases as far as
I can tell.

--
"But Sidley Park is already a picture, and a most amiable picture too.
The slopes are green and gentle. The trees are companionably grouped at
intervals that show them to advantage. The rill is a serpentine ribbon
unwound from the lake peaceably contained by meadows on which the right
amount of sheep are tastefully arranged." -- Lady Croom, "Arcadia"

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 12, 2015

From @tonycoz

On Fri May 08 13​:58​:07 2015, davem wrote​:

So I propose that​:

/abc/o; $foo =~ /$null/

matches $foo against "abc" each time round the loop (it currently
crashes
on the second iteration); and that​:

/abc/; $foo =~ /$null/o

always only evaluates $null once; if that happens to be the empty
string,
then subsequent iterations will see the empty string each time and so
each
time use the last successful match, which may be /abc/ (it currently
ignores the //o on the first iteration if $null is empty).

That makes sense to me.

Or we could take /$null/ meaning 'use the last match' out the back and
bury it.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 12, 2015

From @rjbs

On Fri May 08 13​:58​:07 2015, davem wrote​:

This bug has in fact been present since 5.0 or before; it's just that
a) a change in 5.18.0 made "$_ =~ @​a", where @​a is empty, match "@​a"
(i.e.
"") rather than the string "0"; and b) an assertion added in 5.19.x
made
getting the string value of an AV an error. So I don't think that it
needs
to be a 5.22 blocker any more.

I think I may be missing something here.

It sounds like it's legal to say ($x =~ @​a) to mean ($x =~ "@​a") but that this violates an assertion and so crashes debugging builds. Is this only a non-blocker because it was already broken in 5.20, or for some other reason?

My initial reaction is​: "won't people running debugging builds be irritated that they can't debug code that is legal otherwise?"

As I say, I might be missing something. I don't use debugging builds almost ever.

[...]
My opinion is that, like c and g, the o flag should be honoured on the
match op it appears on, not on the last successful match op, and we
fix
the docs (as well as the code).

This seems fine to me as a v5.23 change.

--
rjbs

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 12, 2015

From @iabyn

On Mon, May 11, 2015 at 08​:20​:17PM -0700, Ricardo SIGNES via RT wrote​:

On Fri May 08 13​:58​:07 2015, davem wrote​:

This bug has in fact been present since 5.0 or before; it's just that
a) a change in 5.18.0 made "$_ =~ @​a", where @​a is empty, match "@​a"
(i.e.
"") rather than the string "0"; and b) an assertion added in 5.19.x
made
getting the string value of an AV an error. So I don't think that it
needs
to be a 5.22 blocker any more.

I think I may be missing something here.

It sounds like it's legal to say ($x =~ @​a) to mean ($x =~ "@​a") but
that this violates an assertion and so crashes debugging builds. Is
this only a non-blocker because it was already broken in 5.20, or for
some other reason?

No, $x =~ @​a is perfectly legal and non-problematic (except that its
semantics changed in 5.18.0, probably inadvertently), both on straight and
debugging builds.

The issue is that on unthreaded builds going back to 5.000, in something
like

  for (....) {
  "a" =~ /./o;
  $foo =~ expr;
  }

where expr evaluates (at least once) to the null string, the optree for
expr will become corrupted, which may affect future iterations of the loop in
unexpected ways. It just so happens that a new debugging assertion added
before 5.20 now detects one of those unexpected things, whereas before it
just silently failed. Having expr be an empty array is just a good way to
trigger this case.

This is why I don't think it should be a 5.22 blocker.

--
I thought I was wrong once, but I was mistaken.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 12, 2015

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2015-05-12T06​:22​:22]

No, $x =~ @​a is perfectly legal and non-problematic (except that its
semantics changed in 5.18.0, probably inadvertently), both on straight and
debugging builds.

The issue is that on unthreaded builds going back to 5.000, in something
like
[...]
This is why I don't think it should be a 5.22 blocker.

Got it. Agreed, thanks.

--
rjbs

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented May 31, 2015

From @ap

* Tony Cook via RT <perlbug-followup@​perl.org> [2015-05-12 02​:30]​:

On Fri May 08 13​:58​:07 2015, davem wrote​:

So I propose that​:

/abc/o; $foo =~ /$null/

matches $foo against "abc" each time round the loop (it currently
crashes
on the second iteration); and that​:

/abc/; $foo =~ /$null/o

always only evaluates $null once; if that happens to be the empty
string,
then subsequent iterations will see the empty string each time and so
each
time use the last successful match, which may be /abc/ (it currently
ignores the //o on the first iteration if $null is empty).

That makes sense to me.

Me too.

--
Aristotle Pagaltzis // <http​://plasmasturm.org/>

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 17, 2016

From @geeknik

perl -e 'sub C0000(){0}C0000 C0000' causes this assertion failure in v5.23.7 (v5.23.6-119-gbc5be89).

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 17, 2016

From [Unknown Contact. See original ticket]

perl -e 'sub C0000(){0}C0000 C0000' causes this assertion failure in v5.23.7 (v5.23.6-119-gbc5be89).

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 19, 2016

From @iabyn

On Mon, May 11, 2015 at 05​:29​:05PM -0700, Tony Cook via RT wrote​:

Or we could take /$null/ meaning 'use the last match' out the back and
bury it.

How about we make a distinction between literal //, and /$null/? Then make
only the latter issue a run-time deprecation warning if $null evaluates to
the empty string?

--
Red sky at night - gerroff my land!
Red sky at morning - gerroff my land!
  -- old farmers' sayings #14

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jan 28, 2016

From @rjbs

* Dave Mitchell <davem@​iabyn.com> [2016-01-19T10​:57​:17]

On Mon, May 11, 2015 at 05​:29​:05PM -0700, Tony Cook via RT wrote​:

Or we could take /$null/ meaning 'use the last match' out the back and
bury it.

How about we make a distinction between literal //, and /$null/? Then make
only the latter issue a run-time deprecation warning if $null evaluates to
the empty string?

So, I bet this was heavily debated back in the year 2011, for #96230... but I
was wrong! (That's when qr// stopped acting like m//.)

Then I guessed it was debated during the discussion of changing "split / /".
Note that with split, the fix was to make "split /$onespace/" act like
"split / /". Again, wrong!

In review, though, I think this won't end up being worth it. Dave's suggestion
(at https://rt-archive.perl.org/perl5/Ticket/Display.html?id=124368#txn-1348595 ) seems
sensible, and there's no pressing /need/ to break this behavior, even if it has
occasionally seemed more trouble than it's worth.

--
rjbs

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 25, 2016

From @geeknik

Triggered in Perl v5.25.5-8-g3c42ae1 with the following​:

./perl -e '$0=s0​::;chop@​0=~y​::​:'
sv.c​:2928​: char *Perl_sv_2pv_flags(SV *const, STRLEN *const, const I32)​: Assertion `((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.
Aborted

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Sep 25, 2016

From [Unknown Contact. See original ticket]

Triggered in Perl v5.25.5-8-g3c42ae1 with the following​:

./perl -e '$0=s0​::;chop@​0=~y​::​:'
sv.c​:2928​: char *Perl_sv_2pv_flags(SV *const, STRLEN *const, const I32)​: Assertion `((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.
Aborted

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 21, 2016

From @jkeenan

On Thu, 23 Apr 2015 09​:31​:36 GMT, brian.carpenter@​gmail.com wrote​:

Built v5.21.12 (v5.21.11-10-ga8f582b) using the following command
line​:

./Configure -des -Dusedevel -DDEBUGGING -Dcc=afl-gcc -Doptimize=-O2\
-g && AFL_HARDEN=1 make -j6 test-prep

Bug found with AFL (http​://lcamtuf.coredump.cx/afl)

This link is now giving a 404. Can someone provide a better link so that we can learn about "AFL"? Thanks.

GDB output​:
perl​: sv.c​:2964​: Perl_sv_2pv_flags​: Assertion `((svtype)((sv)-

sv_flags & 0xff)) != SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) !=
SVt_PVHV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM' failed.

Program received signal SIGABRT, Aborted.
[----------------------------------
registers-----------------------------------]
RAX​: 0x0
RBX​: 0x7fffffffe624 --> 0x736574006c726570 ('perl')
RCX​: 0xffffffffffffffff
RDX​: 0x6
RSI​: 0x126d
RDI​: 0x126d
RBP​: 0x7ffff6ea9c67 --> 0x257325732500203a ('​: ')
RSP​: 0x7fffffffde68 --> 0x7ffff6d933e0 (<*__GI_abort+384>​: mov
rdx,QWORD PTR fs​:0x10)
RIP​: 0x7ffff6d90165 (<*__GI_raise+53>​: cmp rax,0xfffffffffffff000)
R8 : 0x7ffff7fdc700 (0x00007ffff7fdc700)
R9 : 0x5653203d21202929 (')) != SV')
R10​: 0x8
R11​: 0x202
R12​: 0xf3b378 ("((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV &&
((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)-

sv_flags & 0xff)) != SVt_PVFM")
R13​: 0xf4fab0 ("Perl_sv_2pv_flags")
R14​: 0x7ffff6ea9c67 --> 0x257325732500203a ('​: ')
R15​: 0xb94
EFLAGS​: 0x202 (carry parity adjust zero sign trap INTERRUPT direction
overflow)
[-------------------------------------
code-------------------------------------]
0x7ffff6d9015b <*__GI_raise+43>​: movsxd rdi,eax
0x7ffff6d9015e <*__GI_raise+46>​: mov eax,0xea
0x7ffff6d90163 <*__GI_raise+51>​: syscall
=> 0x7ffff6d90165 <*__GI_raise+53>​: cmp rax,0xfffffffffffff000
0x7ffff6d9016b <*__GI_raise+59>​: ja 0x7ffff6d90182
<*__GI_raise+82>
0x7ffff6d9016d <*__GI_raise+61>​: repz ret
0x7ffff6d9016f <*__GI_raise+63>​: nop
0x7ffff6d90170 <*__GI_raise+64>​: test eax,eax
[------------------------------------
stack-------------------------------------]
0000| 0x7fffffffde68 --> 0x7ffff6d933e0 (<*__GI_abort+384>​: mov
rdx,QWORD PTR fs​:0x10)
0008| 0x7fffffffde70 --> 0xf3b378 ("((svtype)((sv)->sv_flags & 0xff))
!= SVt_PVAV && ((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV &&
((svtype)((sv)->sv_flags & 0xff)) != SVt_PVFM")
0016| 0x7fffffffde78 --> 0x7ffff6eabc21 --> 0x706c6568007325 ('%s')
0024| 0x7fffffffde80 --> 0x7fffffffdea0 --> 0x3000000018
0032| 0x7fffffffde88 --> 0xb94
0040| 0x7fffffffde90 --> 0x7fffffffdf90 --> 0x7fffffffe624 -->
0x736574006c726570 ('perl')
0048| 0x7fffffffde98 --> 0x7ffff6dc41b6 (<__fxprintf+310>​: lea
rsp,[rbp-0x20])
0056| 0x7fffffffdea0 --> 0x3000000018
[------------------------------------------------------------------------------
]
Legend​: code, data, rodata, value
Stopped reason​: SIGABRT
0x00007ffff6d90165 in *__GI_raise (sig=<optimized out>)
at ../nptl/sysdeps/unix/sysv/linux/raise.c​:64
64 ../nptl/sysdeps/unix/sysv/linux/raise.c​: No such file or
directory.

System Info​: Debian 7, Kernel 3.2.65-1+deb7u2 x86_64, GCC 4.9.2, libc
2.13-38+deb7u8

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 21, 2016

From @geeknik

I just checked http​://lcamtuf.coredump.cx/afl and it loads just fine with
no errors.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 21, 2016

From @jkeenan

On Mon, 21 Nov 2016 23​:14​:13 GMT, brian.carpenter@​gmail.com wrote​:

I just checked http​://lcamtuf.coredump.cx/afl and it loads just fine with
no errors.

Yes, you are correct. For some reason in my browser the link was picking up the trailing ')', which resulted in the 404.

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Nov 28, 2016

From @hvds

On Sun, 25 Sep 2016 15​:01​:37 -0700, brian.carpenter@​gmail.com wrote​:

Triggered in Perl v5.25.5-8-g3c42ae1 with the following​:

./perl -e '$0=s0​::;chop@​0=~y​::​:'
sv.c​:2928​: char *Perl_sv_2pv_flags(SV *const, STRLEN *const, const
I32)​: Assertion `((svtype)((sv)->sv_flags & 0xff)) != SVt_PVAV &&
((svtype)((sv)->sv_flags & 0xff)) != SVt_PVHV && ((svtype)((sv)-

sv_flags & 0xff)) != SVt_PVFM' failed.
Aborted

This is an unrelated issue. I've created #130198 to track it, with some analysis.

Hugo

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 4, 2017

From @tonycoz

On Fri, 08 May 2015 13​:58​:07 -0700, davem wrote​:

This is fixable, but first we need to agree on what the semantics
should
be, in particular, in the following​:

for (...) {
/abc/o;
/$expr/;
}

on executing /$expr/ if $expr evaluates to the null string, should it
honour the //o of the previous successful match, and so skip
evaluating
$expr on subsequent iterations? This would be hard to implement.

Conversely, should /$expr/o honour the //o flag even if $expr is null?

My opinion is that, like c and g, the o flag should be honoured on the
match op it appears on, not on the last successful match op, and we
fix
the docs (as well as the code).

So I propose that​:

/abc/o; $foo =~ /$null/

matches $foo against "abc" each time round the loop (it currently
crashes
on the second iteration); and that​:

It wasn't clear to me from your post, but this actually behaves differently between threaded and non-threaded builds - for threaded builds it behaves as you describe above - /o is honoured on the op with /o and isn't inherited by and empty match without the /o.

/abc/; $foo =~ /$null/o

always only evaluates $null once; if that happens to be the empty
string,
then subsequent iterations will see the empty string each time and so
each
time use the last successful match, which may be /abc/ (it currently
ignores the //o on the first iteration if $null is empty).

Note that nothing in the test suite currently tests these cases as far
as
I can tell.

The attached covers both these cases, I think.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jul 4, 2017

From @tonycoz

0001-perl-124368-make-foo-o-null-act-consistently.patch
From 154ad615384c3333a75c9915621cac9ddcbd49aa Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Tue, 4 Jul 2017 11:44:06 +1000
Subject: (perl #124368) make /foo/o; /$null/ act consistently

Previously the /o would be inherited by the second match if the first
match was successful, but only on non-threaded builds.

The op-tree rewriting done on non-threaded builds could also confuse
the interpreter, possibly resulting in the match op receiving
the argument intended for the regcomp op.
---
 pp_ctl.c   | 10 ++--------
 t/re/pat.t | 17 ++++++++++++++++-
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/pp_ctl.c b/pp_ctl.c
index db5eabe..d903a4c 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -163,15 +163,9 @@ PP(pp_regcomp)
     /* handle the empty pattern */
     if (!RX_PRELEN(PM_GETRE(pm)) && PL_curpm) {
         if (PL_curpm == PL_reg_curpm) {
-            if (PL_curpm_under) {
-                if (PL_curpm_under == PL_reg_curpm) {
-                    Perl_croak(aTHX_ "Infinite recursion via empty pattern");
-                } else {
-                    pm = PL_curpm_under;
-                }
+            if (PL_curpm_under && PL_curpm_under == PL_reg_curpm) {
+                Perl_croak(aTHX_ "Infinite recursion via empty pattern");
             }
-        } else {
-            pm = PL_curpm;
         }
     }
 
diff --git a/t/re/pat.t b/t/re/pat.t
index fb6d4c4..cf97ecd 100644
--- a/t/re/pat.t
+++ b/t/re/pat.t
@@ -23,7 +23,7 @@ BEGIN {
     skip_all('no re module') unless defined &DynaLoader::boot_DynaLoader;
     skip_all_without_unicode_tables();
 
-plan tests => 837;  # Update this when adding/deleting tests.
+plan tests => 843;  # Update this when adding/deleting tests.
 
 run_tests() unless caller;
 
@@ -138,6 +138,21 @@ sub run_tests {
         $null = "";
         $xyz =~ /$null/;
         is($&, $xyz, $message);
+
+        # each entry: regexp, match string, $&, //o match success
+        my @tests =
+          (
+           [ "", "xy", "x", 1 ],
+           [ "y", "yz", "y", !1 ],
+          );
+        for my $test (@tests) {
+            my ($re, $str, $matched, $omatch) = @$test;
+            $xyz =~ /x/o;
+            ok($str =~ /$re/, "$str matches /$re/");
+            is($&, $matched, "on $matched");
+            $xyz =~ /x/o;
+            is($str =~ /$re/o, $omatch, "$str matches /$re/o (or not)");
+        }
     }
 
     {
-- 
2.1.4

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 14, 2017

From @tonycoz

On Mon, 03 Jul 2017 19​:04​:51 -0700, tonyc wrote​:

On Fri, 08 May 2015 13​:58​:07 -0700, davem wrote​:

This is fixable, but first we need to agree on what the semantics
should
be, in particular, in the following​:

for (...) {
/abc/o;
/$expr/;
}

on executing /$expr/ if $expr evaluates to the null string, should it
honour the //o of the previous successful match, and so skip
evaluating
$expr on subsequent iterations? This would be hard to implement.

Conversely, should /$expr/o honour the //o flag even if $expr is
null?

My opinion is that, like c and g, the o flag should be honoured on
the
match op it appears on, not on the last successful match op, and we
fix
the docs (as well as the code).

So I propose that​:

/abc/o; $foo =~ /$null/

matches $foo against "abc" each time round the loop (it currently
crashes
on the second iteration); and that​:

It wasn't clear to me from your post, but this actually behaves
differently between threaded and non-threaded builds - for threaded
builds it behaves as you describe above - /o is honoured on the op
with /o and isn't inherited by and empty match without the /o.

/abc/; $foo =~ /$null/o

always only evaluates $null once; if that happens to be the empty
string,
then subsequent iterations will see the empty string each time and so
each
time use the last successful match, which may be /abc/ (it currently
ignores the //o on the first iteration if $null is empty).

Note that nothing in the test suite currently tests these cases as
far
as
I can tell.

The attached covers both these cases, I think.

Applied as 3cb4cde.

Tony

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Aug 14, 2017

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

Perl 5.28.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.28.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT
Copy link
Collaborator Author

@p5pRT p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' to 'resolved'

@p5pRT p5pRT closed this Jun 23, 2018
@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
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.