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

Segfault when accessing pad_compname_type #16387

Open
p5pRT opened this issue Jan 27, 2018 · 5 comments
Open

Segfault when accessing pad_compname_type #16387

p5pRT opened this issue Jan 27, 2018 · 5 comments

Comments

@p5pRT
Copy link

p5pRT commented Jan 27, 2018

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

Searchable as RT132774$

@p5pRT
Copy link
Author

p5pRT commented Jan 27, 2018

From konkove@gmail.com

Created by @KES777

When pp_entersub is done we can access to its PAD​:

  SV *sv = PAD_SV( ix );

But when we try to access to pad_compname_type we will get SEGFAULT

For next short script when `test` is called PL_comppad_name will point
to PL_main_cv PAD instead of \&test
sub test {
  my $x; # if you call PAD_COMPNAME( o->op_targ ) at this point you
will get segfault
}

test();

And here if we dump PADnames it will dump PL_main_cv PADNAMES​:
sub test {
  my $x; # Dump padname here
}

my $y
test();

#Dump padname​:
printf( "name​: %s\n", SvPV_nolen( PadnameSV( PAD_COMPNAME( o->op_targ ) ) ) );
This will print $y instead of $x

This error belongs to all macroses which use PAD_COMPNAME(po) macros.
The segfault in first example occurs because PL_comppad_name is not initialized
as PL_curpad, PL_comppad do at pp_hot.c​:5136 (pp_entersub)​:

  PAD_SET_CUR_NOSAVE(padlist, depth);

Looking into this macro (pad.h​:370)​:
#define PAD_SET_CUR_NOSAVE(padlist,nth) \
  PL_comppad = (PAD*) (PadlistARRAY(padlist)[nth]); \
  PL_curpad = AvARRAY(PL_comppad);

We can see that PL_comppad_name is not initialized at all

Here is patch​:
Author​: Eugen Konkov <kes-kes@​yandex.ru>
Date​: Sat Jan 27 00​:39​:26 2018 +0200

  Prevent segfault because of uninitialized PL_comppad_name

Inline Patch
diff --git a/pad.h b/pad.h
index 976dc058d4..4e59a0f30d 100644
--- a/pad.h
+++ b/pad.h
@@ -370,6 +370,7 @@ Restore the old pad saved into the local variable
C by C \#define PAD\_SET\_CUR\_NOSAVE\(padlist\,nth\) \\   PL\_comppad = \(PAD\*\) \(PadlistARRAY\(padlist\)\[nth\]\); \\   PL\_curpad = AvARRAY\(PL\_comppad\); \\ \+ PL\_comppad\_name = \(PadlistNAMES\(padlist\)\); \\   DEBUG\_Xv\(PerlIO\_printf\(Perl\_debug\_log\, \\   "Pad 0x%" UVxf "\[0x%" UVxf "\] set\_cur depth=%d\\n"\, \\   PTR2UV\(PL\_comppad\)\, PTR2UV\(PL\_curpad\)\, \(int\)\(nth\)\)\);

Possible other PAD_* macroses should be reviewed too.

Also here is patch to clarify code a bit​:
Author​: Eugen Konkov <kes-kes@​yandex.ru>
Date​: Sat Jan 27 00​:45​:56 2018 +0200

  PAD_COMPNAME_GEN*​: reuse code from PAD_COMPNAME macros

Inline Patch
diff --git a/pad.h b/pad.h
index 4e59a0f30d..f69a009ead 100644
--- a/pad.h
+++ b/pad.h
@@ -456,11 +456,8 @@ ling pad (lvalue) to C<gen>.
 #define PAD_COMPNAME_OURSTASH(po) \
     (SvOURSTASH(PAD_COMPNAME_SV(po)))
-
+
-#define PAD_COMPNAME_GEN(po) \
-    ((STRLEN)PadnamelistARRAY(PL_comppad_name)[po]->xpadn_gen)
-
-#define PAD_COMPNAME_GEN_set(po, gen) \
-    (PadnamelistARRAY(PL_comppad_name)[po]->xpadn_gen = (gen))
+#define PAD_COMPNAME_GEN(po)             ((STRLEN)PAD_COMPNAME(po)->xpadn_gen)
+#define PAD_COMPNAME_GEN_set(po, gen)    (PAD_COMPNAME(po)->xpadn_gen = (gen))
-
+
-
+
 /*
Perl Info

Flags:
    category=core
    severity=critical

Site configuration information for perl 5.24.1:

Configured by kes at Sun Apr 30 22:40:30 EEST 2017.

Summary of my perl5 (revision 5 version 24 subversion 1) configuration:

  Platform:
    osname=linux, osvers=4.4.0-53-generic, archname=x86_64-linux
    uname='linux kes-x751sa 4.4.0-53-generic #74-ubuntu smp fri dec 2
15:59:10 utc 2016 x86_64 x86_64 x86_64 gnulinux '
    config_args='-de
-Dprefix=/home/kes/perl5/perlbrew/perls/perl-5.24.1
-Aeval:scriptdir=/home/kes/perl5/perlbrew/perls/perl-5.24.1/bin'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64',
    optimize='-O2',
    cppflags='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include'
    ccversion='', gccversion='5.4.0 20160609', 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='cc', ldflags =' -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/local/lib
/usr/lib/gcc/x86_64-linux-gnu/5/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.23.so, so=so, useshrplib=false, libperl=libperl.a
    gnulibc_version='2.23'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -L/usr/local/lib
-fstack-protector-strong'

Locally applied patches:
    Devel::PatchPerl 1.38


@INC for perl 5.24.1:
    /home/kes/perl5/perlbrew/perls/perl-5.24.1/lib/site_perl/5.24.1/x86_64-linux
    /home/kes/perl5/perlbrew/perls/perl-5.24.1/lib/site_perl/5.24.1
    /home/kes/perl5/perlbrew/perls/perl-5.24.1/lib/5.24.1/x86_64-linux
    /home/kes/perl5/perlbrew/perls/perl-5.24.1/lib/5.24.1


Environment for perl 5.24.1:
    HOME=/home/kes
    LANG=cinnamon.desktop
    LANGUAGE=en_US
    LC_ALL=en_US.UTF-8
    LC_MESSAGES=en_US.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/kes/perl5/perlbrew/bin:/home/kes/perl5/perlbrew/perls/perl-5.24.1/bin:/home/kes/bin:/home/kes/bin:/home/kes/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games
    PERLBREW=command perlbrew
    PERLBREW_BASHRC_VERSION=0.78
    PERLBREW_HOME=/home/kes/.perlbrew
    PERLBREW_MANPATH=/home/kes/perl5/perlbrew/perls/perl-5.24.1/man
    PERLBREW_PATH=/home/kes/perl5/perlbrew/bin:/home/kes/perl5/perlbrew/perls/perl-5.24.1/bin
    PERLBREW_PERL=perl-5.24.1
    PERLBREW_ROOT=/home/kes/perl5/perlbrew
    PERLBREW_VERSION=0.78
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2018

From zefram@fysh.org

Eugen Konkov wrote​:

But when we try to access to pad_compname_type we will get SEGFAULT

Not a bug. pad_compname_type() is explicitly documented to refer to
the *currently-compiling* pad; it's not for the runtime pad. Likewise,
PL_comppad_name is only documented to be meaningful during compilation.
Initialising PL_comppad_name during ordinary runtime would be a waste
of effort. If you really want access to the pad names for the currently
executing sub, you can get there via CvPADLIST.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2018

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

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2018

From @KES777

PL_comppad and PL_comppad_name are named similarly, so they should have similar behavior

It will be handy to have one PL_curcv and
PL_curcv.curpad, PL_curcv.padnames, PL_curcv.depth, PL_curcv.pads(N) (where PL_curcv.pads(0) will return .curpad)

Also PL_curcv maybe returned from find_runcv(0)

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2018

From @iabyn

On Tue, Jan 30, 2018 at 02​:02​:25AM -0800, KES via RT wrote​:

PL_comppad and PL_comppad_name are named similarly, so they should have
similar behavior

That is not a valid reason for changing the existing behaviour.

It will be handy to have one PL_curcv and
PL_curcv.curpad, PL_curcv.padnames, PL_curcv.depth, PL_curcv.pads(N) (where PL_curcv.pads(0) will return .curpad)

Also PL_curcv maybe returned from find_runcv(0)

All that extra state would incur overhead on each function call and
return.

On the other hand, I've been considering for a while implementing a
PL_curcv var, but it would be a CV* rather than what you suggest. The
cost of saving/restoring it would be weighed against currently having to
save/restore PL_comppad. Also, getting the context of the last expression
in a sub would become cheaper. This happens quite a lot.

--
In my day, we used to edit the inodes by hand. With magnets.

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

3 participants