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

Double macro argument evaluation in S_init_main_stash #16292

Closed
p5pRT opened this issue Dec 8, 2017 · 8 comments
Closed

Double macro argument evaluation in S_init_main_stash #16292

p5pRT opened this issue Dec 8, 2017 · 8 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Dec 8, 2017

Migrated from rt.perl.org#132545 (status was 'resolved')

Searchable as RT132545$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 8, 2017

From pipcet@gmail.com

Created by pipcet@gmail.com

I just came across the following problem​:

S_init_main_stash contains the code​:

  PL_curstash = PL_defstash = (HV *)SvREFCNT_inc_simple_NN(newHV());

SvREFCNT_inc_simple_NN is defined as​:

#define SvREFCNT_inc_simple_NN(sv) (++(SvREFCNT(sv)),MUTABLE_SV(sv))

That evaluates its argument twice, which breaks with side effects.

So newHV() is called twice, one of the new HVs gets refcount 2 and
gets leaked, the other gets refcount 1 and gets stored in two
places. I suspect this is low-priority, but there might be a way to
actually destroy the stash while it is still stored in PL_defstash, in
which case it's a user-visible bug.

Sorry if this message gets duplicated, I tried sending it a few days ago
and it appears to have vanished.

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl 5.27.7:

Configured by pip at Mon Dec  4 13:27:33 UTC 2017.

Summary of my perl5 (revision 5 version 27 subversion 7) configuration:

  Platform:
    osname=linux
    osvers=4.13.0-1-amd64
    archname=x86_64-linux
    uname='linux amygdala 4.13.0-1-amd64 #1 smp debian 4.13.10-1
(2017-10-30) x86_64 gnulinux '
    config_args=''
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='g++'
    ccflags ='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -D_LARGEFILE_SOURCE
-D_FILE_OFFSET_BITS=64 -I/home/pip/git/emacs-c++/js/dist/include
-pthread -g3 -ggdb'
    optimize='-O2'
    cppflags='-fwrapv -fno-strict-aliasing -pipe
-fstack-protector-strong -I/usr/local/include -pthread'
    ccversion=''
    gccversion='8.0.0 20171124 (experimental)'
    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='ld'
    ldflags =' -fstack-protector-strong -g3 -ggdb -L/usr/local/lib'
    libpth=/usr/local/lib/gcc/x86_64-pc-linux-gnu/8.0.0/../../../../include/c++/8.0.0
/usr/local/lib/gcc/x86_64-pc-linux-gnu/8.0.0/../../../../include/c++/8.0.0/x86_64-pc-linux-gnu
/usr/local/lib/gcc/x86_64-pc-linux-gnu/8.0.0/../../../../include/c++/8.0.0/backward
/usr/local/lib /usr/local/lib/gcc/x86_64-pc-linux-gnu/8.0.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 /lib64
/usr/lib64 /usr/local/lib64
    libs=-lpthread -pthread -lnsl -ldl -lm -lcrypt -lutil -lc
-lmozjs-58a1 -Wl,--whole-archive mozglue.a -Wl,--no-whole-archive
    perllibs=-lpthread -pthread -lnsl -ldl -lm -lcrypt -lutil -lc -L
/home/pip/git/emacs-c++/js/dist/bin -lmozjs-58a1 -Wl,--whole-archive
/home/pip/git/emacs-c++/js/mozglue/build/libmozglue.a
-Wl,--no-whole-archive
    libc=libc-2.25.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version='2.25'
  Dynamic Linking:
    dlsrc=dl_none.xs
    dlext=none
    d_dlsymun=undef
    ccdlflags=''
    cccdlflags=''
    lddlflags=''



@INC for perl 5.27.7:
    .
    lib
    /usr/local/lib/perl5/site_perl/5.27.7/x86_64-linux
    /usr/local/lib/perl5/site_perl/5.27.7
    /usr/local/lib/perl5/5.27.7/x86_64-linux
    /usr/local/lib/perl5/5.27.7


Environment for perl 5.27.7:
    HOME=/home/pip
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH=/home/pip/git/emacs-c++/js/dist/bin:/home/pip/amd/AMDAPPSDK-3.0/lib/x86_64/
    LOGDIR (unset)
    PATH=/home/pip/.cargo/bin:/home/pip/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:/sbin:/usr/local/sbin:/usr/sbin:/sbin:/usr/local/sbin:/usr/sbin
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 8, 2017

From @arc

Thanks very much for your report. I believe this is fixed as of commit 9842f1a.

--
Aaron Crane

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 8, 2017

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

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 8, 2017

@arc - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Dec 8, 2017
@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 8, 2017

From @cpansprout

On Fri, 08 Dec 2017 02​:28​:38 -0800, arc wrote​:

Thanks very much for your report. I believe this is fixed as of commit
9842f1a.

Thank you for the quick fix. I was guilty of making that mistake, in this commit​:

commit 03d9f02
Author​: Father Chrysostomos <sprout@​cpan.org>
Date​: Sat Oct 22 11​:06​:35 2011 -0700

  [perl #101486] Make PL_curstash refcounted
 
  This stops PL_curstash from pointing to a freed-and-reused scalar in
  cases like ‘package Foo; BEGIN {*Foo​:: = *Bar​::}’.
 
  In such cases, another BEGIN block, or any subroutine definition,
  would cause a crash. Now it just happily proceeds. newATTRSUB and
  newXS have been modified not to call mro_method_changed_in in such
  cases, as it doesn’t make sense.

This was when I added refcounting to PL_curstash to begin with. So I wonder whether it is possible to test the fix.

--

Father Chrysostomos

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 10, 2017

From @hvds

On Fri, 08 Dec 2017 02​:28​:38 -0800, arc wrote​:

Thanks very much for your report. I believe this is fixed as of commit
9842f1a.

Is there also some portable way to fix the double-evaluation of the macro, or can we use some naming convention to call out the macros that will do multiple evaluation?

Are we at the point where our target set of compilers can all handle inline functions, for example?

Hugo

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 10, 2017

From pipcet@gmail.com

On Fri, Dec 8, 2017 at 2​:36 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

This was when I added refcounting to PL_curstash to begin with. So I wonder whether it is possible to test the fix.

I don't think it is. The code a few lines down from where the bug was
increments PL_defstash's refcount again, for *INC, which is
unfreeable, so PL_curstash never noticed that its refcount was one
less than it should have been.

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Dec 10, 2017

From zefram@fysh.org

Hugo van der Sanden via RT wrote​:

Is there also some portable way to fix the double-evaluation of the
macro, or can we use some naming convention to call out the macros that
will do multiple evaluation?

We do the latter, to some extent. The SvREFCNT_inc() macros with
"_simple" in their name are permitted to double evaluate​: the "_simple"
indicates that the caller guarantees that the argument is simple enough.
So another way to fix this particular bug would have been to drop the
"_simple". With some other macros we have the convention of an "x"
suffix to indicate that the macro will not double evaluate​: for example,
SvIVx() as the single-evaluation version of SvIV().

Are we at the point where our target set of compilers can all handle
inline functions, for example?

They can all handle static functions, which is close enough.

-zefram

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

1 participant