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

protect some magic %main:: symbols from being deleted #12908

Closed
p5pRT opened this issue Apr 9, 2013 · 14 comments
Closed

protect some magic %main:: symbols from being deleted #12908

p5pRT opened this issue Apr 9, 2013 · 14 comments

Comments

@p5pRT
Copy link

p5pRT commented Apr 9, 2013

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

Searchable as RT117543$

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2013

From @rurban

Created by @rurban

This is a bug report for perl from rurban@​cpanel.net,
generated with the help of perlbug 1.39 running under perl 5.17.8.

-----------------------------------------------------------------
Deleting some symbols form the %main​:: namespace will cause perl to
segfault with certain ops.

  perl -e 'delete $​::{ENV}; chdir' # by hugmeir
  perl -e 'delete $​::{"\010"}; eval ""' # by hugmeir

Adding this to S_hv_delete_common()

  if ( hv == PL_defstash ) {
  /* Some main symbols may not be deleted */
  if ( (klen == 3 && memEQ(key, "ENV", 3)) /* needed e.g. for chdir */
  || (klen == 1 && *key == 8) ) /* for eval */
  croak("Attempt to delete $​::{%s}", key);
  }

and so on solves it, without taking into account that \010 needs to be
properly escaped and we shouldn't waste too many perldiag entries for it.
So it needs to be​:

  if ( hv == PL_defstash ) {
  /* Some main symbols may not be deleted */
  if ( (klen == 3 && memEQ(key, "ENV", 3)) /* needed e.g. for chdir */
  || (klen == 1 && *key == 8) ) /* for eval */
  croak("Attempt to delete $​::{%s}",
  *key < 32 ? pv_display(newSVpvs_flags("", *SVs_TEMP),key,klen,0,255)
  : key);
  }

Probably other symbols need also to be protected,
like STDOUT, STDERR for do_curse() (also by hugmeir)

Perl Info

Flags:
    category=core
    severity=high

This perlbug was built using Perl 5.17.8 - Fri Feb  1 11:00:49 CST 2013
It is being executed now by  Perl 5.17.8 - Wed Jan  9 17:52:45 CST 2013.

Site configuration information for perl 5.17.8:

Configured by rurban at Wed Jan  9 17:52:45 CST 2013.

Summary of my perl5 (revision 5 version 17 subversion 8) configuration:
  Commit id: 1e9a14d0d069d64df71ad32c7174ede653a57801
  Platform:
    osname=linux, osvers=3.2.0-4-amd64, archname=x86_64-linux-thread-multi-debug
    uname='linux reini 3.2.0-4-amd64 #1 smp debian 3.2.32-1 x86_64 gnulinux '
    config_args='-de -Dusedevel -Uversiononly -Dinstallman1dir=none -Dinstallman3dir=none -Dinstallsiteman1dir=none -Dinstallsiteman3dir=none -DEBUGGING -Doptimize=-g3 -Duseithreads -Accflags='-msse4.2' -Accflags='-march=corei7' -Dcf_email='rurban@cpanel.net' -Dperladmin='rurban@cpanel.net' -Duseshrplib'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -msse4.2 -march=corei7 -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-g3',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -msse4.2 -march=corei7 -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.7.2', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib/x86_64-linux-gnu /lib/../lib /usr/lib/x86_64-linux-gnu /usr/lib/../lib /lib /usr/lib
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc -lgdbm_compat
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.13'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/lib/perl5/5.17.8/x86_64-linux-thread-multi-debug/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -g3 -L/usr/local/lib -fstack-protector'

Locally applied patches:
    


@INC for perl 5.17.8:
    /usr/local/lib/perl5/site_perl/5.17.8/x86_64-linux-thread-multi-debug
    /usr/local/lib/perl5/site_perl/5.17.8
    /usr/local/lib/perl5/5.17.8/x86_64-linux-thread-multi-debug
    /usr/local/lib/perl5/5.17.8
    /usr/local/lib/perl5/site_perl/5.17.7
    /usr/local/lib/perl5/site_perl/5.17.6
    /usr/local/lib/perl5/site_perl/5.17.5
    /usr/local/lib/perl5/site_perl/5.17.4
    /usr/local/lib/perl5/site_perl/5.17.3
    /usr/local/lib/perl5/site_perl/5.17.2
    /usr/local/lib/perl5/site_perl/5.17.1
    /usr/local/lib/perl5/site_perl/5.17.0
    /usr/local/lib/perl5/site_perl/5.17
    /usr/local/lib/perl5/site_perl/5.16.2
    /usr/local/lib/perl5/site_perl/5.16.1
    /usr/local/lib/perl5/site_perl/5.16.0
    /usr/local/lib/perl5/site_perl/5.15.9
    /usr/local/lib/perl5/site_perl/5.15.8
    /usr/local/lib/perl5/site_perl/5.15.7
    /usr/local/lib/perl5/site_perl/5.15.6
    /usr/local/lib/perl5/site_perl/5.15.5
    /usr/local/lib/perl5/site_perl/5.15.4
    /usr/local/lib/perl5/site_perl/5.14.3
    /usr/local/lib/perl5/site_perl/5.14.2
    /usr/local/lib/perl5/site_perl/5.14.1
    /usr/local/lib/perl5/site_perl/5.12.4
    /usr/local/lib/perl5/site_perl/5.10.1
    /usr/local/lib/perl5/site_perl/5.8.9
    /usr/local/lib/perl5/site_perl/5.8.8
    /usr/local/lib/perl5/site_perl/5.8.7
    /usr/local/lib/perl5/site_perl/5.8.6
    /usr/local/lib/perl5/site_perl/5.8.5
    /usr/local/lib/perl5/site_perl/5.8.4
    /usr/local/lib/perl5/site_perl/5.8.3
    /usr/local/lib/perl5/site_perl/5.8.2
    /usr/local/lib/perl5/site_perl/5.8.1
    /usr/local/lib/perl5/site_perl/5.6.2
    /usr/local/lib/perl5/site_perl
    .


Environment for perl 5.17.8:
    HOME=/home/rurban
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH=/usr/src/perl/blead/perl-git
    LOGDIR (unset)
    PATH=/home/rurban/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2013

From @rurban

See my github branch rurban/delete-protected-syms-rt117543 for the
current work, protecting ENV and \010.

https://github.com/rurban/perl/tree/rurban/delete-protected-syms-rt117543

Ongoing research what other syms need to be protected like that.
--
Reini Urban

@p5pRT
Copy link
Author

p5pRT commented Apr 9, 2013

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

@p5pRT
Copy link
Author

p5pRT commented Apr 10, 2013

From @Hugmeir

On Tue, Apr 9, 2013 at 1​:24 PM, rurban@​cpanel.net <perlbug-followup@​perl.org

wrote​:

# New Ticket Created by rurban@​cpanel.net
# Please include the string​: [perl #117543]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=117543 >

This is a bug report for perl from rurban@​cpanel.net,
generated with the help of perlbug 1.39 running under perl 5.17.8.

-----------------------------------------------------------------
Deleting some symbols form the %main​:: namespace will cause perl to
segfault with certain ops.

perl -e 'delete $​::{ENV}; chdir' # by hugmeir
perl -e 'delete $​::{"\010"}; eval ""' # by hugmeir

Adding this to S_hv_delete_common()

if \( hv == PL\_defstash \) \{
    /\* Some main symbols may not be deleted \*/
    if \( \(klen == 3 && memEQ\(key\, "ENV"\, 3\)\) /\* needed e\.g\. for chdir

*/
|| (klen == 1 && *key == 8) ) /* for eval */
croak("Attempt to delete $​::{%s}", key);
}

and so on solves it, without taking into account that \010 needs to be
properly escaped and we shouldn't waste too many perldiag entries for it.
So it needs to be​:

I'm not sure if this is the right solution; You can probably still make it
crash by temporarily replacing PL_defstash.

if \( hv == PL\_defstash \) \{
    /\* Some main symbols may not be deleted \*/
    if \(   \(klen == 3 && memEQ\(key\, "ENV"\, 3\)\) /\* needed e\.g\. for

chdir */
|| (klen == 1 && *key == 8) ) /* for eval */
croak("Attempt to delete $​::{%s}",
*key < 32 ? pv_display(newSVpvs_flags("",
*SVs_TEMP),key,klen,0,255)
: key);
}

Probably other symbols need also to be protected,
like STDOUT, STDERR for do_curse() (also by hugmeir)

$^R (PL_replgv) for regexen, *_ (PL_defgv) for everything (it won't crash,
but it'll start exposing strange implementation details; try loading Carp
as an example), *CORE​::GLOBAL​:: (PL_globalstash) for overrides (e.g.
require, do).

Here's the thing though. There's a handful of variables where this does not
happen​:

perl -e 'delete $​::{INC}; require strict;' # All good, even though @​INC and
%INC were supposedly gone.
perl -e 'delete $​::{q{@​}}; eval {}' # Still good

This is because S_init_main_stash in perl.c has lines like these​:

SvREFCNT_inc_simple_void(PL_incgv); /* Don't allow it to be freed */

Which will internally preserve *INC, even though it's not being exposed to
the user anymore. Perhaps the variables that perl needs to work should be
protected likewise. This alone would solve most cases where this ticket
rears its ugly head. To make it even safer, a complementary fix can
safeguard each location that uses a magic variable, to make sure that they
autovivify the GVs they need.

Here's a proof of concept of the above​:
https://github.com/Hugmeir/utf8mess/tree/safeguard_variables

@p5pRT
Copy link
Author

p5pRT commented Apr 10, 2013

From @nwc10

On Wed, Apr 10, 2013 at 07​:29​:05AM -0300, Brian Fraser wrote​:

Here's the thing though. There's a handful of variables where this does not
happen​:

perl -e 'delete $​::{INC}; require strict;' # All good, even though @​INC and
%INC were supposedly gone.
perl -e 'delete $​::{q{@​}}; eval {}' # Still good

This is because S_init_main_stash in perl.c has lines like these​:

SvREFCNT_inc_simple_void(PL_incgv); /* Don't allow it to be freed */

Which will internally preserve *INC, even though it's not being exposed to
the user anymore. Perhaps the variables that perl needs to work should be
protected likewise. This alone would solve most cases where this ticket
rears its ugly head. To make it even safer, a complementary fix can

But it arguably contradicts the documentation further. Right now, require
isn't *quite* doing what is documented. In that, I can have @​INC set, as
verified from Perl space, but require fail​:

$ perl -le '@​COPY = @​INC; @​INC = (); delete $​::{INC}; eval q{@​INC = @​COPY}; eval q{print foreach @​INC}; require strict'
/opt/local/lib/perl5/site_perl/5.12.4/darwin-thread-multi-2level
/opt/local/lib/perl5/site_perl/5.12.4
/opt/local/lib/perl5/vendor_perl/5.12.4/darwin-thread-multi-2level
/opt/local/lib/perl5/vendor_perl/5.12.4
/opt/local/lib/perl5/5.12.4/darwin-thread-multi-2level
/opt/local/lib/perl5/5.12.4
/opt/local/lib/perl5/site_perl/5.12.3
/opt/local/lib/perl5/site_perl
/opt/local/lib/perl5/vendor_perl
.
Can't locate strict.pm in @​INC (@​INC contains​:) at -e line 1.

Now, to be clear I'm not saying that the above is anything other than a
contrived test case. But my point is that if the interpreter squirrels away
pointers to the start-up values of various typeglobs, and then the squirrelled
away version can get detached from the version referred to by name, then it's
not actually really doing what the documentation says.

safeguard each location that uses a magic variable, to make sure that they
autovivify the GVs they need.

This should be sufficient, without the reference count increase games,
although sv_clear needs to be sure to NULL things out if it finds that they
are being freed.

In that doing this, it becomes possible to "reattach" the shortcut pointers
to the things that they represent, and the pointers are only shortcuts.
Not first class privileged owners that can't be override.

But I guess this comes down to a design decision - what are those pointers
really about. Are they a speedup, where the desired semantic remains that
the interpreter behaves as if it's looking things up in %​:: as it needs them?
Or are they a specified part of the implementation, in that the interpreter's
direct pointers are *initially* made available in %​::, but if Perl-space is
careless it can loose its access to them?

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2013

From @tonycoz

On Wed Apr 10 04​:29​:03 2013, nicholas wrote​:

But I guess this comes down to a design decision - what are those
pointers
really about. Are they a speedup, where the desired semantic remains
that
the interpreter behaves as if it's looking things up in %​:: as it
needs them?
Or are they a specified part of the implementation, in that the
interpreter's
direct pointers are *initially* made available in %​::, but if Perl-
space is
careless it can loose its access to them?

I'd mostly prefer to protect symbols, but I wonder if that would
prevent interesting uses[1] of the current implementation.

We have three choices I can think of​:

1) retain the current implementation - the user can crash perl from
pure perl, but there might be interesting things the user can do
with that power.

2) protect any globals connected to PL_* from deletion

3) ensure PL_* are updated when their perl variables are updated.

I can see 2) or 3) having a performance impact if they cover all the
variables we want to protect (if we do it by name.)

I don't really have an opinion either way.

Tony

(Looking at this ticket because it has patches.)

[1] none of which I can think of right now

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2013

From @davidnicol

We have three choices I can think of​:

1) retain the current implementation - the user can crash perl from
pure perl, but there might be interesting things the user can do
with that power.

2) protect any globals connected to PL_* from deletion

3) ensure PL_* are updated when their perl variables are updated.

I'm a little bit stunned that nobody yet has suggested fixing the
operations that cause
the segfaults so they will fail gracefully when these variables that are
normally there have been removed. Locking down these magic globals seems,
as they say, incompletely gluteated.

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2013

From @Leont

On Wed, Jul 17, 2013 at 6​:55 PM, David Nicol <davidnicol@​gmail.com> wrote​:

I'm a little bit stunned that nobody yet has suggested fixing the operations
that cause
the segfaults so they will fail gracefully when these variables that are
normally there have been removed. Locking down these magic globals seems, as
they say, incompletely gluteated.

I think Brian suggested just that when he talked about "safeguard each
location that uses a magic variable", and others discussed some
complications with that approach.

Leon

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2013

From @bulk88

I've attached what I think is a a pure perl example of this bug, run with 5.10.0. Correct me if it isn't an example.


C​:\Documents and Settings\Owner\Desktop>perl stashdelete.pl
$@​ not equal to glob SCALAR at stashdelete.pl line 11.

C​:\Documents and Settings\Owner\Desktop>perl stashdelete.pl

C​:\Documents and Settings\Owner\Desktop>


1st run is with the "delete $​::{'@​'};" active. 2nd is with "delete $​::{'@​'};" commented out.

Now a "my $walker = B​::Concise​::compile('-src','-exec', *run{CODE});"


C​:\Documents and Settings\Owner\Desktop>perl stashdelete.pl
B​::Concise​::compile(CODE(0x182a53c))
# 13​: eval {dienow()};
1 <;> nextstate(main 352 stashdelete.pl​:13) v​:{
2 <|> entertry(other->3) v
# 13​: eval {dienow()};
r <;> nextstate(main 351 stashdelete.pl​:13) v
s <0> pushmark s
t <#> gv[*dienow] s
u <1> entersub[t2] vKS/TARG,1
3 <@​> leavetry vK
# 14​: die '$@​ is broken' if index($@​,"die now") != 0;
4 <;> nextstate(main 352 stashdelete.pl​:14) v​:{
5 <#> gvsv[*@​] s
6 <$> const[GV "die now"] s
7 <@​> index[t5] sK/2
8 <$> const[IV 0] s
9 <2> ne sK/2
a <|> and(other->b) vK/1
b <0> pushmark s
c <$> const[PV "$@​ is broken"] s
d <@​> die[t3] vK/1
# 15​: die '$@​ not equal to glob SCALAR ' if $@​ ne ${ *{'main​::@​'}{SCALAR} };
e <;> nextstate(main 354 stashdelete.pl​:15) v​:{
f <#> gvsv[*@​] s
g <$> const[PV "main​::@​"] s
h <1> rv2gv sKR/1
i <$> const[PV "SCALAR"] s/BARE
j <2> gelem sK/2
k <1> rv2sv sK/1
l <2> sne sK/2
m <|> and(other->n) K/1
n <0> pushmark s
o <$> const[PV "$@​ not equal to glob SCALAR "] s
p <@​> die[t6] sK/1
q <1> leavesub[1 ref] K/REFC,1

C​:\Documents and Settings\Owner\Desktop>


--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2013

From @bulk88

stashdelete.pl

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2013

From @bulk88

On Sat Dec 21 16​:55​:06 2013, bulk88 wrote​:

I've attached what I think is a a pure perl example of this bug, run
with 5.10.0. Correct me if it isn't an example.

Wrong file uploaded. Uping right file.

--
bulk88 ~ bulk88 at hotmail.com

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2013

From @bulk88

sub dienow {
    die "die now";
}
use Data::Dumper;

#comment delete out to make this pass
#delete $::{'@'};

eval {dienow()};
die '$@ is broken' if index($@,"die now") != 0;
die '$@ not equal to glob SCALAR ' if $@ ne ${ *{'main::@'}{SCALAR} };

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2013

@cpansprout

bulk88 wrote​:

I've attached what I think is a a pure perl example of this bug, run with
5.10.0. Correct me if it isn't an example.

I think delete $​::{'@​'} falls squarely under the 'don't do that' cat-
egory. But at least it should not result in memory leaks or crashes.
I also think this ticket was about the latter (crashes), rather than
about the behaviour of delete $​::{'@​'} per se.

I believe I have fixed all crashes of this sort.

@toddr
Copy link
Member

toddr commented Jan 31, 2020

I have tested all of the reported cases and they do not fail on 5.30. I'm closing this case as resolved.

@toddr toddr closed this as completed Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants