CopSTASH can point to freed-and-reused SV

p5pRT commented 12 years ago

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

Searchable as RT113486$

p5pRT commented 12 years ago

From @cpansprout

On non-threaded builds\, cops have a direct pointer to their stash\, which is not reference-counted.

caller returns undef in that case:

$ ./perl -ILib -e 'package foo { sub bar { main::bar() } } sub bar { delete $::{"foo::"}; warn scalar caller }; foo::bar' Warning: something's wrong at -e line 1.

But it returns undef by accident. cop_stash is pointing to a freed scalar\, which is not SvOOK\, so HvNAME_HEK returns false.

I haven’t come up with a test case yet\, but the freed scalar could be reused for another stash\, giving erroneous results. Or it could be used for a scalar with the offset hack applied\, which would result in crashes.

In fixing another bug\, there is a chance I will extend this bug to threaded perls\, too.

This is how I know the scalar is freed:

$ gdb --args ./perl -ILib -e 'package foo { sub bar { main::bar() } } sub bar { delete $::{"foo::"}; warn caller }; foo::bar' GNU gdb 6.3.50-20050815 (Apple version gdb-1469) (Wed May 5 04:30:06 UTC 2010) Copyright 2004 Free Software Foundation\, Inc. GDB is free software\, covered by the GNU General Public License\, and you are welcome to change it and/or distribute copies of it under certain conditions. Type "show copying" to see the conditions. There is absolutely no warranty for GDB. Type "show warranty" for details. This GDB was configured as "i386-apple-darwin"...Reading symbols for shared libraries .... done

(gdb) break Perl_pp_caller Breakpoint 1 at 0x17225f: file pp_ctl.c\, line 1877. (gdb) run Starting program: /Users/sprout/Perl/perl.git-copy/perl -ILib -e package\ foo\ \{\ sub\ bar\ \{\ main::bar\ \}\ \}\ sub\ bar\ \{\ delete\ \$::\{\"foo::\"\}\;\ warn\ caller\ \}\;\ foo::bar Reading symbols for shared libraries +++. done

Breakpoint 1\, Perl_pp_caller () at pp_ctl.c:1877 1877 dSP; (gdb) n Current language: auto; currently c++ 1883 bool has_arg = MAXARG && TOPs; (gdb) 1891 cx = caller_cx(count + !!(PL_op->op_private & OPpOFFBYONE)\, &dbcx); (gdb) 1892 if (!cx) { (gdb) 1900 stash_hek = HvNAME_HEK((HV*)CopSTASH(cx->blk_oldcop)); (gdb) 1901 if (GIMME != G_ARRAY) { (gdb) call Perl_sv_dump( cx->cx_u.cx_blk.blku_oldcop->cop_stash) SV = UNKNOWN(0xff) (0xabababab) at 0x821b70 REFCNT = -1414812757 FLAGS = () (gdb)

p5pRT commented 12 years ago

From @cpansprout

On Sun Jun 03 18:32:35 2012\, sprout wrote:

On non-threaded builds\, cops have a direct pointer to their stash\, which is not reference-counted.

caller returns undef in that case:

$ ./perl -ILib -e 'package foo { sub bar { main::bar() } } sub bar { delete $::{"foo::"}; warn scalar caller }; foo::bar' Warning: something's wrong at -e line 1.

But it returns undef by accident. cop_stash is pointing to a freed scalar\, which is not SvOOK\, so HvNAME_HEK returns false.

I haven’t come up with a test case yet\, but the freed scalar could be reused for another stash\, giving erroneous results.

I’m wondering whether this is even worth fixing.

For non-threaded perls\, it would require modifying any remaining op trees when freeing hashes. It would probably then require every stash to have symtab magic attached to it.

Under threads\, we would have to make cops hold a refcount on the PL_stashpad slot so we know when it can be reused.

Having cops hold a refcount on the stash would result in too many circular references\, so that’s a not a good option.

Or it could be used for a scalar with the offset hack applied\, which would result in crashes.

{ package foo; sub bar { main::bar() } } sub bar { delete $::{"foo::"}; my $x = \($1+2); my $y = \($1+2); # this is the one that reuses the mem addr\, but my $z = \($1+2); # try the others just in case s/2// for $$x\, $$y\, $$z; # now SvOOK warn scalar caller # boom }; foo::bar

I’ve fixed that in commit e788621.

In fixing another bug\, there is a chance I will extend this bug to threaded perls\, too.

Which I have done. But fixing that other bug actually fixed about three bugs at once.

--

Father Chrysostomos

p5pRT commented 12 years ago

From [Unknown Contact. See original ticket]

On Sun Jun 03 18:32:35 2012\, sprout wrote:

On non-threaded builds\, cops have a direct pointer to their stash\, which is not reference-counted.

caller returns undef in that case:

$ ./perl -ILib -e 'package foo { sub bar { main::bar() } } sub bar { delete $::{"foo::"}; warn scalar caller }; foo::bar' Warning: something's wrong at -e line 1.

But it returns undef by accident. cop_stash is pointing to a freed scalar\, which is not SvOOK\, so HvNAME_HEK returns false.

I haven’t come up with a test case yet\, but the freed scalar could be reused for another stash\, giving erroneous results.

I’m wondering whether this is even worth fixing.

For non-threaded perls\, it would require modifying any remaining op trees when freeing hashes. It would probably then require every stash to have symtab magic attached to it.

Under threads\, we would have to make cops hold a refcount on the PL_stashpad slot so we know when it can be reused.

Having cops hold a refcount on the stash would result in too many circular references\, so that’s a not a good option.

Or it could be used for a scalar with the offset hack applied\, which would result in crashes.

{ package foo; sub bar { main::bar() } } sub bar { delete $::{"foo::"}; my $x = \($1+2); my $y = \($1+2); # this is the one that reuses the mem addr\, but my $z = \($1+2); # try the others just in case s/2// for $$x\, $$y\, $$z; # now SvOOK warn scalar caller # boom }; foo::bar

I’ve fixed that in commit e788621.

In fixing another bug\, there is a chance I will extend this bug to threaded perls\, too.

Which I have done. But fixing that other bug actually fixed about three bugs at once.

--

Father Chrysostomos

p5pRT commented 12 years ago

@cpansprout - Status changed from 'new' to 'open'

p5pRT commented 7 years ago

From zefram@fysh.org

Father Chrysostomos wrote:

On non-threaded builds\, cops have a direct pointer to their stash\, which is not reference-counted.

We could address this by giving every stash one extra counted ref upon initialisation\, so that it can never be freed. This is cheap and wouldn't change any API. The downside is that the stash would leak. I don't make a habit of deleting stashes\, so I don't know whether this would be significant. Is anyone likely to delete an unbounded number of them in one program run? It seems to me that stash deletion is a troublesome operation anyway\, conflicting with the concept of absolute package names.

-zefram

p5pRT commented 7 years ago

From @cpansprout

On Nov 15\, 2017\, at 10:38 PM\, Zefram via RT \perlbug\-followup@perl\.org wrote:

Father Chrysostomos wrote:

On non-threaded builds\, cops have a direct pointer to their stash\, which is not reference-counted.

We could address this by giving every stash one extra counted ref upon initialisation\, so that it can never be freed. This is cheap and wouldn't change any API. The downside is that the stash would leak. I don't make a habit of deleting stashes\, so I don't know whether this would be significant. Is anyone likely to delete an unbounded number of them in one program run?

See: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=75176

It seems to me that stash deletion is a troublesome operation anyway\, conflicting with the concept of absolute package names.

Perl / perl5