Taint-mode assert fail in Perl_magic_clearisa without other symptoms

[p5pRT]

Migrated from rt.perl.org#128254 (status was 'open')

Searchable as RT128254$

[p5pRT]

From @dcollinsn

Greetings Porters\,

I have compiled bleadperl with the afl-gcc compiler using​:

./Configure -Dusedevel -Dprefix='/usr/local/perl-afl' -Dcc='ccache afl-gcc' -Uuselongdouble -Duse64bitall -Doptimize=-g -Uversiononly -Uman1dir -Uman3dir -Dusequadmath -des AFL_HARDEN=1 make && make test

And then fuzzed the resulting binary using​:

AFL_NO_VAR_CHECK=1 afl-fuzz -i in -o out bin/perl @​@​

After reducing testcases using `afl-tmin` and performing additional minimization by hand\, I have located the following testcase that triggers an assert fail in debug buids of the perl interpreter. The testcase is the file below. On normal builds\, this runs normally (albeit with an expected warning). On debug builds\, this returns an assert fail.

dcollins@​nightshade64​:\~/perl$ ./perl -Ilib -tW -e '{@​{*a​::ISA}=undef*a​::ISA;@​a​::ISA=0}' Use of uninitialized value in list assignment at -e line 1. dcollins@​nightshade64​:\~/perl$ cd ../perldebug/

dcollins@​nightshade64​:\~/perldebug$ ./perl -Ilib -tW -e '{@​{*a​::ISA}=undef*a​::ISA;@​a​::ISA=0}' Use of uninitialized value in list assignment at -e line 1. perl​: mg.c​:1726​: Perl_magic_clearisa​: Assertion `((((_gvstash)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((_gvstash)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((_gvstash)->sv_flags & 0xff)) == SVt_PVLV))' failed. Aborted

Debugging tool output is below. A git bisect was performed and reported the following.

5e267fb87fe17363909b2ed49f916dccf9939c3b is the first bad commit commit 5e267fb87fe17363909b2ed49f916dccf9939c3b Author​: David Mitchell \davem@​iabyn\.com Date​: Wed Oct 21 13​:10​:44 2015 +0100

  Always copy return values when exiting scope

  v5.14.0-642-g3ed94dc fixed certain instances where returning from a sub   incorrectly returned the actual value rather than a copy\, e.g.

  sub f { delete $foo{bar} }

  This was because if the value(s) being returned had SvTEMP set\, copying   was skipped. That commit added an extra condition to the skip test\,   SvREFCNT(sv) == 1.

  However\, this applies equally well to other scope exits\, for example

  do { ...; delete $foo{bar} }

  So this commits adds the RC==1 test to S_leave_common() too\, which handles   all the non-sub scope exits. As well as adding a test to do.t\, it adds an   additional test to sub.t\, since the original tests\, although they   *detected* a non-copied return\, didn't actually demonstrate a case where   it was actually harmful.

  Note that S_leave_common() also sometimes skips on PADTMPs as well as   TEMPs\, so this commit as a side-effect also makes it copy PADTMPs unless   their RC ==1. But since their RC should in fact almost always be 1 anyway\,   in practice it makes no difference.

:100644 100644 82189bb5c188e77800176fe8cfcb80b15bcb2226 d1229af7609c41d1caa7944f8d90eb6a2b12859c M pp_ctl.c :040000 040000 95d71e14d5e47e4db9a5149492215aa45b9ad6c8 f69fbca5ed4bd5f0595f03be7deda11cca858d43 M t bisect run success

**GDB**

dcollins@​nightshade64​:\~/perldebug$ gdb --args ./perl -Ilib -tW -e '{@​{*a​::ISA}=undef*a​::ISA;@​a​::ISA=0}' GNU gdb (GDB) 7.10 Copyright (C) 2015 Free Software Foundation\, Inc. License GPLv3+​: GNU GPL version 3 or later \<http​://gnu.org/licenses/gpl.html> This is free software​: you are free to change and redistribute it. There is NO WARRANTY\, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-unknown-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions\, please see​: \<http​://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at​: \<http​://www.gnu.org/software/gdb/documentation/>. For help\, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from ./perl...done. (gdb) run Starting program​: /home/dcollins/perldebug/perl -Ilib -tW -e \{@​\{\*a​::ISA\}=undef\*a​::ISA\;@​a​::ISA=0\} [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". Use of uninitialized value in list assignment at -e line 1. perl​: mg.c​:1726​: Perl_magic_clearisa​: Assertion `((((_gvstash)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((_gvstash)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((_gvstash)->sv_flags & 0xff)) == SVt_PVLV))' failed.

Program received signal SIGABRT\, Aborted. 0x00007ffff6cf9478 in raise () from /lib/x86_64-linux-gnu/libc.so.6 (gdb) bt #0 0x00007ffff6cf9478 in raise () from /lib/x86_64-linux-gnu/libc.so.6 #1 0x00007ffff6cfa8fa in abort () from /lib/x86_64-linux-gnu/libc.so.6 #2 0x00007ffff6cf23a7 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #3 0x00007ffff6cf2452 in __assert_fail () from /lib/x86_64-linux-gnu/libc.so.6 #4 0x000000000057548f in Perl_magic_clearisa (sv=0x0\, mg=0xabc3e0) at mg.c​:1724 #5 0x0000000000575104 in Perl_magic_setisa (sv=0xab20b0\, mg=0xabc3e0) at mg.c​:1691 #6 0x000000000056dfa1 in Perl_mg_set (sv=0xab20b0) at mg.c​:277 #7 0x00000000005aeacc in Perl_pp_aassign () at pp_hot.c​:1422 #8 0x000000000055a245 in Perl_runops_debug () at dump.c​:2239 #9 0x00000000004623d3 in S_run_body (oldscope=1) at perl.c​:2517 #10 0x00000000004619fe in perl_run (my_perl=0xa9c010) at perl.c​:2440 #11 0x000000000041eae0 in main (argc=5\, argv=0x7fffffffe5f8\, env=0x7fffffffe628)   at perlmain.c​:116 (gdb) f 4 #4 0x000000000057548f in Perl_magic_clearisa (sv=0x0\, mg=0xabc3e0) at mg.c​:1724 1724 stash = GvSTASH( (gdb) l 1719 } 1720 1721 return 0; 1722 } 1723 1724 stash = GvSTASH( 1725 (const GV *)mg->mg_obj 1726 ); 1727 1728 /* The stash may have been detached from the symbol table\, so check its (gdb) info locals _gvstash = 0xab2068 stash = 0xab20b0 __PRETTY_FUNCTION__ = "Perl_magic_clearisa"

**VALGRIND**

No reported memory management errors.

**PERL -V**

dcollins@​nightshade64​:\~/perldebug$ ./perl -Ilib -V Summary of my perl5 (revision 5 version 25 subversion 2) configuration​:   Commit id​: c29dfc6a6c45f86648c51f961304254cc3c449b9   Platform​:   osname=linux\, osvers=4.5.0-2-amd64\, archname=x86_64-linux-ld   uname='linux nightshade64 4.5.0-2-amd64 #1 smp debian 4.5.3-2 (2016-05-08) x86_64 gnulinux '   config_args='-Dusedevel -Dprefix=/usr/local/perl-afl -Dcc=ccache gcc-6.1 -Duselongdouble -Duse64bitall -Doptimize=-g -Uversiononly -Uman1dir -Uman3dir -DDEBUGGING -DPERL_POISON -des'   hint=recommended\, useposix=true\, d_sigaction=define   useithreads=undef\, usemultiplicity=undef   use64bitint=define\, use64bitall=define\, uselongdouble=define   usemymalloc=n\, bincompat5005=undef   Compiler​:   cc='ccache gcc-6.1'\, ccflags ='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'\,   optimize='-g'\,   cppflags='-fwrapv -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'   ccversion=''\, gccversion='6.1.0'\, 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='long double'\, nvsize=16\, Off_t='off_t'\, lseeksize=8   alignbytes=16\, prototype=define   Linker and Libraries​:   ld='ccache gcc-6.1'\, ldflags =' -fstack-protector-strong -L/usr/local/lib'   libpth=/usr/local/lib /usr/local/lib/gcc/x86_64-pc-linux-gnu/6.1.0/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.22.so\, so=so\, useshrplib=false\, libperl=libperl.a   gnulibc_version='2.22'   Dynamic Linking​:   dlsrc=dl_dlopen.xs\, dlext=so\, d_dlsymun=undef\, ccdlflags='-Wl\,-E'   cccdlflags='-fPIC'\, lddlflags='-shared -g -L/usr/local/lib -fstack-protector-strong'

Characteristics of this binary (from libperl)​:   Compile-time options​: DEBUGGING HAS_TIMES PERLIO_LAYERS PERL_COPY_ON_WRITE   PERL_DONT_CREATE_GVSV   PERL_HASH_FUNC_ONE_AT_A_TIME_HARD PERL_MALLOC_WRAP   PERL_OP_PARENT PERL_PRESERVE_IVUV PERL_USE_DEVEL   USE_64_BIT_ALL USE_64_BIT_INT USE_LARGE_FILES   USE_LOCALE USE_LOCALE_COLLATE USE_LOCALE_CTYPE   USE_LOCALE_NUMERIC USE_LOCALE_TIME USE_LONG_DOUBLE   USE_PERLIO USE_PERL_ATOF   Built under linux   Compiled at May 26 2016 17​:57​:37   @​INC​:   lib   /usr/local/perl-afl/lib/site_perl/5.25.2/x86_64-linux-ld   /usr/local/perl-afl/lib/site_perl/5.25.2   /usr/local/perl-afl/lib/5.25.2/x86_64-linux-ld   /usr/local/perl-afl/lib/5.25.2   /usr/local/perl-afl/lib/site_perl/5.25.1   /usr/local/perl-afl/lib/site_perl/5.24.0   /usr/local/perl-afl/lib/site_perl   .

[p5pRT]

From @iabyn

On Thu\, May 26\, 2016 at 05​:29​:21PM -0700\, Dan Collins wrote​:

dcollins@​nightshade64​:\~/perldebug$ ./perl -Ilib -tW -e '{@​{*a​::ISA}=undef*a​::ISA;@​a​::ISA=0}' Use of uninitialized value in list assignment at -e line 1. perl​: mg.c​:1726​: Perl_magic_clearisa​: Assertion `((((_gvstash)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((_gvstash)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((_gvstash)->sv_flags & 0xff)) == SVt_PVLV))' failed. Aborted

It's equivalent to the following​:

perl -t run against​:

  undef *a​::ISA;   @​{*a​::ISA}=();   @​a​::ISA=0

The problem is in the weak pointer from ISA magic to the GV\, i.e. where the *a​::ISA points to the GP which points to @​a​::ISA which has ISA magic attached\, who's mg_obj field contains a weak pointer back to *a​::ISA. It has to be weak\, otherwise there would be a reference loop and leakage.

However\, with this​: @​{*a​::ISA}\, the *a​::ISA expression is executed within a block scope; when that scope is left\, a copy of the scope's return value is returned (as is done with all rvalue scope exits). This copying creates an SvTEMP GV whose GP is shared with the original *a​::ISA GV. When the gp_av slot of the temp GV is accessed\, it is empty due to the earlier undef\, so is autovivified. The autovivification sees that the GV's name is 'ISA' so attaches ISA magic\, whose mg_obj field is set to point (weakly) to the temp GV copy of *a​::ISA.

At the next statement boundary temps are freed\, and @​a​::ISA's mg_obj field now points to a freed GV. Crashes etc follow.

I don't know how to fix this.

-- Dave's first rule of Opera​: If something needs saying\, say it​: don't warble it.

[p5pRT]

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

[p5pRT]

From zefram@fysh.org

Dave Mitchell wrote​:

I don't know how to fix this.

The GP has a pointer to the interned (`effective') GV from which any GV referencing it was cloned. Attaching ISA magic should follow that pointer.

-zefram

[p5pRT]

From @cpansprout

On Mon Jun 27 08​:54​:23 2016\, davem wrote​:

It's equivalent to the following​:

perl -t run against​:

undef *a​::ISA; @​{*a​::ISA}=(); @​a​::ISA=0

Why does it only happen under -t and -T?

--

Father Chrysostomos

[p5pRT]

From @iabyn

On Mon\, Jun 27\, 2016 at 05​:58​:15PM -0700\, Father Chrysostomos via RT wrote​:

On Mon Jun 27 08​:54​:23 2016\, davem wrote​:

It's equivalent to the following​:

perl -t run against​:

undef *a​::ISA; @​{*a​::ISA}=(); @​a​::ISA=0

Why does it only happen under -t and -T?

I just checked. In the absence of -t\, the @​{*a​::ISA} is compiled with a scope rather than enter/leave. Changing it to something like @​{my $x; *a​::ISA} to force full scope\, causes it to crash without the -t.

-- Any [programming] language that doesn't occasionally surprise the novice will pay for it by continually surprising the expert.   -- Larry Wall

[p5pRT]

From @iabyn

On Mon\, Jun 27\, 2016 at 05​:27​:05PM +0100\, Zefram wrote​:

Dave Mitchell wrote​:

I don't know how to fix this.

The GP has a pointer to the interned (`effective') GV from which any GV referencing it was cloned. Attaching ISA magic should follow that pointer.

That won't work in a case like​:

  *Bar​::ISA = *Foo​::ISA

When the ISA array is modified\, both stashes need mro_isa_changed_in() called on them\, but there's only a single GP involved.

At the moment\, mg_obj normally points to the ISA AV's GV\, but when there are multiple GVs\, as in the above or *Bar​::ISA = \@​Foo​::ISA\, then mg_obj points to a non-reified AV whose elements are the parent GVs.

But the current scheme will (I imagine) fail in the case of something like

  $Bar​::{ISA} = $Foo​::{ISA};

Perhaps instead of storing GV or GP pointer(s) in the ISA magic\, we should store stash names (i.e. a PV or an AV of PV's). Then when @​ISA is modified\, the stashes are looked up from the stash names.

-- Little fly\, thy summer's play my thoughtless hand has terminated with extreme prejudice.   (with apologies to William Blake)

[p5pRT]

From @cpansprout

On Tue Jun 28 02​:35​:26 2016\, davem wrote​:

On Mon\, Jun 27\, 2016 at 05​:27​:05PM +0100\, Zefram wrote​:

Dave Mitchell wrote​:

I don't know how to fix this.

The GP has a pointer to the interned (`effective') GV from which any GV referencing it was cloned. Attaching ISA magic should follow that pointer.

That won't work in a case like​:

\*Bar​::ISA = \*Foo​::ISA

When the ISA array is modified\, both stashes need mro_isa_changed_in() called on them\, but there's only a single GP involved.

At the moment\, mg_obj normally points to the ISA AV's GV\, but when there are multiple GVs\, as in the above or *Bar​::ISA = \@​Foo​::ISA\, then mg_obj points to a non-reified AV whose elements are the parent GVs.

But the current scheme will (I imagine) fail in the case of something like

$Bar​::\{ISA\} = $Foo​::\{ISA\};

It’s practically impossible for perl to track things correctly when people do direct stash assignment. (That’s partly why perlmod says​:

The results of creating new symbol table entries directly or modifying any entries that are not already typeglobs are undefined and subject to change between releases of perl.

)

So as long as that case does not crash\, I think that’s fine.

Perhaps instead of storing GV or GP pointer(s) in the ISA magic\, we should store stash names (i.e. a PV or an AV of PV's). Then when @​ISA is modified\, the stashes are looked up from the stash names.

As long as things still work when stashes get effectively renamed (Bar gets renamed in *Foo​::=*Bar​::\, *Bar​::=*Baz​::). I went to great lengths to make sure that worked (in 5.12\, iirc)\, and I would hate to see it break.

--

Father Chrysostomos