From @hvds

Created by @hvds

The check to "Assert valid op_private bits in op_free()" added by Dave
in d0c8136 can be triggered by the temporary poking of numargs into
op_private by ck_fun(), if a syntax error causes unwinding before the
field has been reset (presumably by a later ck_* function).

My shortest test case is​:
% ./miniperl -e 'map+map'
Not enough arguments for map at -e line 1, at EOF
Execution of -e aborted due to compilation errors.
miniperl​: op.c​:721​: Perl_op_free​: Assertion `!(o->op_private & ~PL_op_private_valid[type])' failed.
Aborted (core dumped)
%

In eb8433b, Nick added a FIXME comment asking whether the storing of
numargs should move after the "too many args" check, but that wouldn't
help us in this case - first because it's the later "too few args" check
we fail on, and second because the mapstart op we assert on isn't the one
for which we've generated the "Not enough arguments" error.

I'm not sure how to search for what uses the numargs; ideally we'd remove
the need to piggyback on op_private in this way.

(I've been trying out American Fuzzy Lop​: this represents a high proportion
of the failure cases I've seen so far. It also finds cases triggering the
same assert for different reasons on an RV2CV, but I haven't looked at
those yet.)

Flags:
    category=core
    severity=medium

Site configuration information for perl 5.21.9:

Configured by hv at Fri Feb  6 16:50:59 GMT 2015.

Summary of my perl5 (revision 5 version 21 subversion 9) configuration:
  Commit id: eb82332cb71f48a5a63aa48dda0f6f55ee333ecb
  Platform:
    osname=linux, osvers=3.13.0-37-generic, archname=x86_64-linux
    uname='linux shad2 3.13.0-37-generic #64-ubuntu smp mon sep 22 21:28:38 utc 2014 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Dcc=gcc -Dprefix=/opt/blead-d -Doptimize=-g -O6 -DDEBUGGING -Dusedevel -Uversiononly'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-g -O6',
    cppflags='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.8.2', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678, doublekind=3
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16, longdblkind=3
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /usr/lib/gcc/x86_64-linux-gnu/4.8/include-fixed /usr/include/x86_64-linux-gnu /usr/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib
    libs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc
    libc=libc-2.19.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.19'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -g -O6 -L/usr/local/lib -fstack-protector'



@INC for perl 5.21.9:
    /opt/blead-d/lib/perl5/site_perl/5.21.9/x86_64-linux
    /opt/blead-d/lib/perl5/site_perl/5.21.9
    /opt/blead-d/lib/perl5/5.21.9/x86_64-linux
    /opt/blead-d/lib/perl5/5.21.9
    .


Environment for perl 5.21.9:
    HOME=/home/hv
    LANG=C
    LANGUAGE=en_GB:en
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/hv/bin:/home/hv/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
    PERL_BADLANG (unset)
    SHELL=/bin/bash

From @cpansprout

On Fri Feb 06 08​:59​:56 2015, hv wrote​:

I'm not sure how to search for what uses the numargs;

Anything using MAXARG uses that. Most ops that take one or two arguments already have the lower bits set to the number of operands by newUNOP or newBINOP. But index, for instance, depends on ck_fun to set those bits.

--

Father Chrysostomos

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

From @cpansprout

On Fri Feb 06 08​:59​:56 2015, hv wrote​:

This is a bug report for perl from hv@​crypt.org,
generated with the help of perlbug 1.40 running under perl 5.21.9.

-----------------------------------------------------------------
[Please describe your issue here]

The check to "Assert valid op_private bits in op_free()" added by Dave
in d0c8136 can be triggered by the temporary poking of numargs into
op_private by ck_fun(), if a syntax error causes unwinding before the
field has been reset (presumably by a later ck_* function).

My shortest test case is​:
% ./miniperl -e 'map+map'
Not enough arguments for map at -e line 1, at EOF
Execution of -e aborted due to compilation errors.
miniperl​: op.c​:721​: Perl_op_free​: Assertion `!(o->op_private &
~PL_op_private_valid[type])' failed.
Aborted (core dumped)
%

In eb8433b, Nick added a FIXME comment asking whether the storing
of
numargs should move after the "too many args" check, but that wouldn't
help us in this case - first because it's the later "too few args"
check
we fail on, and second because the mapstart op we assert on isn't the
one
for which we've generated the "Not enough arguments" error.

I think it’s the assertion that’s wrong. Usually ck_fun sets the number of args, but ck_grep clears it. If an error occurs, it does not clear it (and there is no way to guarantee that it will, as any yyerror may croak), so the bit is left set. All we need to do is change the bit assignments in regen/op_private, which I plan to do soon.

--

Father Chrysostomos

From @cpansprout

On Fri Feb 06 10​:02​:51 2015, sprout wrote​:

On Fri Feb 06 08​:59​:56 2015, hv wrote​:

This is a bug report for perl from hv@​crypt.org,
generated with the help of perlbug 1.40 running under perl 5.21.9.

-----------------------------------------------------------------
[Please describe your issue here]

The check to "Assert valid op_private bits in op_free()" added by
Dave
in d0c8136 can be triggered by the temporary poking of numargs
into
op_private by ck_fun(), if a syntax error causes unwinding before the
field has been reset (presumably by a later ck_* function).

My shortest test case is​:
% ./miniperl -e 'map+map'
Not enough arguments for map at -e line 1, at EOF
Execution of -e aborted due to compilation errors.
miniperl​: op.c​:721​: Perl_op_free​: Assertion `!(o->op_private &
~PL_op_private_valid[type])' failed.
Aborted (core dumped)
%

In eb8433b, Nick added a FIXME comment asking whether the storing
of
numargs should move after the "too many args" check, but that
wouldn't
help us in this case - first because it's the later "too few args"
check
we fail on, and second because the mapstart op we assert on isn't the
one
for which we've generated the "Not enough arguments" error.

I think it’s the assertion that’s wrong. Usually ck_fun sets the
number of args, but ck_grep clears it. If an error occurs, it does
not clear it (and there is no way to guarantee that it will, as any
yyerror may croak), so the bit is left set. All we need to do is
change the bit assignments in regen/op_private, which I plan to do
soon.

I’ve fixed this in fb0c7c3. I was not able to come up with any cases that failed with other lower bits set. Do the other cases you mentioned still fail?

--

Father Chrysostomos

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

From @hvds

On Fri Feb 06 10​:17​:02 2015, sprout wrote​:

I’ve fixed this in fb0c7c3. I was not able to come up with any
cases that failed with other lower bits set. Do the other cases you
mentioned still fail?

Heh, I was about to say no, but more careful checking shows that only 12 of the 20 RV2CV cases now pass.

I'm attaching to the ticket a tar file of the 8 that still fail - they all contain nul bytes and similar oddities. The 22 mapstart cases all pass now.

Hugo

From @hvds

rv2cv-assert.tar

From @cpansprout

On Fri Feb 06 10​:43​:18 2015, hv wrote​:

On Fri Feb 06 10​:17​:02 2015, sprout wrote​:

I’ve fixed this in fb0c7c3. I was not able to come up with any
cases that failed with other lower bits set. Do the other cases you
mentioned still fail?

Heh, I was about to say no, but more careful checking shows that only
12 of the 20 RV2CV cases now pass.

I'm attaching to the ticket a tar file of the 8 that still fail - they
all contain nul bytes and similar oddities. The 22 mapstart cases all
pass now.

The first of those is definitely memory corruption. It crashes intermittently. If I dump the op before the assertion I get a different set of private flags each time.

--

Father Chrysostomos

From @hvds

On Fri Feb 06 12​:56​:03 2015, sprout wrote​:

The first of those is definitely memory corruption.

Ah, I have caught something in the act​:

(gdb) cont
Hardware watchpoint 3​: ((UNOP*)0xa81898)->op_private

Old value = 0 '\000'
New value = 25 '\031'
Perl_newUNOP (type=17, flags=11016408, first=0xa81918) at op.c​:4811
4811 unop = (UNOP*) CHECKOP(type, unop);
(gdb) # ^^^^^ so it triggered on 'unop->op_private = (U8)(1 | (flags >> 8));'
(gdb) p /x flags
$22 = 0xa818d8
(gdb) # ^^^^^ that's almost certainly an OP address, not flags
(gdb) where
#0 Perl_newUNOP (type=17, flags=11016408, first=0xa81918) at op.c​:4811
#1 0x0000000000446b80 in Perl_newCVREF (flags=11016408, o=0xa81918)
  at op.c​:9377
#2 0x00000000004d309a in Perl_yyparse (gramtype=258) at perly.y​:1105
#3 0x000000000045f79a in S_parse_body (env=0x0, xsinit=0x41ef3b <xs_init>)
  at perl.c​:2273
#4 0x000000000045dde7 in perl_parse (my_perl=0xa5d010,
  xsinit=0x41ef3b <xs_init>, argc=2, argv=0x7fffffffe658, env=0x0)
  at perl.c​:1607
#5 0x000000000041ee9d in main (argc=2, argv=0x7fffffffe658,
  env=0x7fffffffe670) at perlmain.c​:114

So we're managing to call newCVREF via this bit of perly​:
  amper : '&' indirob
  { $$ = newCVREF($1,$2); }
.. but $1 is an op address instead of a sane set of flags. (Note that there also seems to be some reuse of op bodies going on​: the same watchpoint was getting triggered by the Zero() at op.c​:250.)

For ease of reference, here's a hex dump of the program​:
--- id​:000001,sig​:11,src​:001079,op​:havoc,rep​:8
000000 22 61 62 7d 22 61 78 3b 26 00 7a 8a 6f 7d 82 78 "ab}"ax;&.z.o}.x
000010 3b 0a ;.

It'll take me a while to dig further, I haven't had to pretend not to be scared of perly.y for a good 10 years. But hey, I think that means the new assert has found something really useful. :)

From @cpansprout

On Fri Feb 06 12​:56​:03 2015, sprout wrote​:

The first of those is definitely memory corruption. It crashes
intermittently. If I dump the op before the assertion I get a
different set of private flags each time.

No, not memory corruption.

Perl’s parser tolerates stray nulls, effectively treating them as whitespace, or so I thought....

$ perl -e 'print qq|use strict; "a";"a";"a";&\0e|' | ./perl -Ilib
Undefined subroutine &main​::e called at - line 1.
$ perl -e 'print qq|use strict; "a";"a";"a";"a";&\0e|' | ./perl -Ilib
Bareword "e" not allowed while "strict subs" in use at - line 1.
Execution of - aborted due to compilation errors.

$ perl -e 'print qq|&\0eq|' | ./perl -Ilib
syntax error at - line 1, near "&eq"
Execution of - aborted due to compilation errors.

There the lexer produces '&' EQOP instead of '&' WORD.

A memory address (the last value in pl_yylval) is being stuffed into the flags field, which explains the intermittency. Usually it is harmless (it happens for &{...}, which seems to be impervious to it), but &\0foo results in unexpected code paths being followed. The nulls should probably be skipped when the identifier after & is scanned.

--

Father Chrysostomos

From @cpansprout

On Fri Feb 06 17​:21​:30 2015, hv wrote​:

On Fri Feb 06 12​:56​:03 2015, sprout wrote​:

The first of those is definitely memory corruption.

Ah, I have caught something in the act​:

(gdb) cont
Hardware watchpoint 3​: ((UNOP*)0xa81898)->op_private

Old value = 0 '\000'
New value = 25 '\031'
Perl_newUNOP (type=17, flags=11016408, first=0xa81918) at op.c​:4811
4811 unop = (UNOP*) CHECKOP(type, unop);
(gdb) # ^^^^^ so it triggered on 'unop->op_private = (U8)(1 | (flags

8));'
(gdb) p /x flags
$22 = 0xa818d8
(gdb) # ^^^^^ that's almost certainly an OP address, not flags
(gdb) where
#0 Perl_newUNOP (type=17, flags=11016408, first=0xa81918) at
op.c​:4811
#1 0x0000000000446b80 in Perl_newCVREF (flags=11016408, o=0xa81918)
at op.c​:9377
#2 0x00000000004d309a in Perl_yyparse (gramtype=258) at perly.y​:1105
#3 0x000000000045f79a in S_parse_body (env=0x0, xsinit=0x41ef3b
<xs_init>)
at perl.c​:2273
#4 0x000000000045dde7 in perl_parse (my_perl=0xa5d010,
xsinit=0x41ef3b <xs_init>, argc=2, argv=0x7fffffffe658, env=0x0)
at perl.c​:1607
#5 0x000000000041ee9d in main (argc=2, argv=0x7fffffffe658,
env=0x7fffffffe670) at perlmain.c​:114

So we're managing to call newCVREF via this bit of perly​:
amper : '&' indirob
{ $$ = newCVREF($1,$2); }
.. but $1 is an op address instead of a sane set of flags. (Note that
there also seems to be some reuse of op bodies going on​: the same
watchpoint was getting triggered by the Zero() at op.c​:250.)

For ease of reference, here's a hex dump of the program​:
--- id​:000001,sig​:11,src​:001079,op​:havoc,rep​:8
000000 22 61 62 7d 22 61 78 3b 26 00 7a 8a 6f 7d 82 78
"ab}"ax;&.z.o}.x
000010 3b 0a
;.

It'll take me a while to dig further, I haven't had to pretend not to
be scared of perly.y for a good 10 years. But hey, I think that means
the new assert has found something really useful. :)

It’s the PREREF('&') in yylex that is not setting pl_yylval before returning. But, as I pointed out in my other message, &\0foo should ideally not even reach that code path.

--

Father Chrysostomos

From @cpansprout

On Fri Feb 06 18​:13​:31 2015, sprout wrote​:

On Fri Feb 06 17​:21​:30 2015, hv wrote​:

On Fri Feb 06 12​:56​:03 2015, sprout wrote​:

The first of those is definitely memory corruption.

Ah, I have caught something in the act​:

(gdb) cont
Hardware watchpoint 3​: ((UNOP*)0xa81898)->op_private

Old value = 0 '\000'
New value = 25 '\031'
Perl_newUNOP (type=17, flags=11016408, first=0xa81918) at op.c​:4811
4811 unop = (UNOP*) CHECKOP(type, unop);
(gdb) # ^^^^^ so it triggered on 'unop->op_private = (U8)(1 | (flags

8));'
(gdb) p /x flags
$22 = 0xa818d8
(gdb) # ^^^^^ that's almost certainly an OP address, not flags
(gdb) where
#0 Perl_newUNOP (type=17, flags=11016408, first=0xa81918) at
op.c​:4811
#1 0x0000000000446b80 in Perl_newCVREF (flags=11016408, o=0xa81918)
at op.c​:9377
#2 0x00000000004d309a in Perl_yyparse (gramtype=258) at perly.y​:1105
#3 0x000000000045f79a in S_parse_body (env=0x0, xsinit=0x41ef3b
<xs_init>)
at perl.c​:2273
#4 0x000000000045dde7 in perl_parse (my_perl=0xa5d010,
xsinit=0x41ef3b <xs_init>, argc=2, argv=0x7fffffffe658, env=0x0)
at perl.c​:1607
#5 0x000000000041ee9d in main (argc=2, argv=0x7fffffffe658,
env=0x7fffffffe670) at perlmain.c​:114

So we're managing to call newCVREF via this bit of perly​:
amper : '&' indirob
{ $$ = newCVREF($1,$2); }
.. but $1 is an op address instead of a sane set of flags. (Note that
there also seems to be some reuse of op bodies going on​: the same
watchpoint was getting triggered by the Zero() at op.c​:250.)

For ease of reference, here's a hex dump of the program​:
--- id​:000001,sig​:11,src​:001079,op​:havoc,rep​:8
000000 22 61 62 7d 22 61 78 3b 26 00 7a 8a 6f 7d 82 78
"ab}"ax;&.z.o}.x
000010 3b 0a
;.

It'll take me a while to dig further, I haven't had to pretend not to
be scared of perly.y for a good 10 years. But hey, I think that means
the new assert has found something really useful. :)

It’s the PREREF('&') in yylex that is not setting pl_yylval before
returning. But, as I pointed out in my other message, &\0foo should
ideally not even reach that code path.

I have fixed this in 3c47da3. I still want to investigate whether stuffing an address into the flags still causes problems (mainly because I want to add tests for it if I fix it).

--

Father Chrysostomos

From @cpansprout

On Sat Feb 07 10​:11​:05 2015, sprout wrote​:

On Fri Feb 06 18​:13​:31 2015, sprout wrote​:

It’s the PREREF('&') in yylex that is not setting pl_yylval before
returning. But, as I pointed out in my other message, &\0foo should
ideally not even reach that code path.

I have fixed this in 3c47da3. I still want to investigate whether
stuffing an address into the flags still causes problems (mainly
because I want to add tests for it if I fix it).

I have managed to trigger an assertion failure with &{+foo}, which I fixed in eea8938.

--

Father Chrysostomos

From @hvds

On Sat Feb 07 10​:11​:05 2015, sprout wrote​:

I have fixed this in 3c47da3. I still want to investigate whether
stuffing an address into the flags still causes problems (mainly
because I want to add tests for it if I fix it).

I've restarted AFL with latest blead and a couple of tweaks, and it's doing much better; but after 9 hours it came up with this​:

% hd crashes/id*
--- id​:000000,sig​:11,src​:001996,op​:havoc,rep​:8
000000 61 24 24 24 3f 26 24 24 24 24 24 10 24 24 3b 19 a$$$?&$$$$$.$$;.
000010 24 $
%

Its afl-tmin tool minimized that to this​:

% hd ~/crash
--- /home/hv/crash
000000 24 30 3f 26 24 30 10 $0?&$0.
% /opt/blead-d0/bin/perl ~/crash
Unrecognized character \x10; marked by <-- HERE after $0?&$0<-- HERE near column 7 at /home/hv/crash line 1.
% /opt/blead-d0/bin/perl ~/crash
Unrecognized character \x10; marked by <-- HERE after $0?&$0<-- HERE near column 7 at /home/hv/crash line 1.
perl​: op.c​:721​: Perl_op_free​: Assertion `!(o->op_private & ~PL_op_private_valid[type])' failed.
Aborted (core dumped)
%

.. which looks like it's probably the same issue.

Hugo

From @hvds

On Sat Feb 07 20​:39​:09 2015, hv wrote​:

.. which looks like it's probably the same issue.

.. and which looks to be fixed by eea8938. (Sorry, I hadn't seen the update when I sent that.)

Hugo

From @khwilliamson

Thanks for submitting this ticket

The issue should be resolved with the release today of Perl v5.22, available at http​://www.perl.org/get.html
If you find that the problem persists, feel free to reopen this ticket

--
Karl Williamson for the Perl 5 porters team

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