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.

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

From @arc

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

--
Aaron Crane

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

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

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

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

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.

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