open ${\$x} leaks

[p5pRT]

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

Searchable as RT115814$

[p5pRT]

From @cpansprout

If you write open $fh or open $hash{key}, the autogenerated handle is not added to the symbol table, so undef $fh or undef %hash will free it.

If you wriet open ${\$fh}, it *does* get added to the symbol table, so it leaks.

Is there any guarantee that a handle created like that will be accessible like this?

  $name = "".*$fh;
  $fh = *$name;

Another thing​:

A handle created via open $hash{key} is given the name __ANONIO__. A handle added to the symbol table is given a name like _GEN_0. The latter could be useful for debugging purposes, because each handle would have a different name. Should __ANONIO__ be changed to _GEN_0?

newGVgen, which creates a new handle (with a name like _GEN_0), also adds it to the symbol table. It is an API function. Should it stop adding it to the symbol table? How much of CPAN would break? I started to look, and what I have seen so far would not break, but it was only about 20 distributions.

---

Flags​:
  category=core
  severity=low

---

Site configuration information for perl 5.17.6​:

Configured by sprout at Sat Nov 17 23​:09​:15 PST 2012.

Summary of my perl5 (revision 5 version 17 subversion 6) configuration​:
  Local Commit​: e1d8196c98d7cc6d34eedc83ba4193885673d43d
  Ancestor​: 10ffaf9
  Platform​:
  osname=darwin, osvers=10.5.0, archname=darwin-thread-multi-2level
  uname='darwin pint.local 10.5.0 darwin kernel version 10.5.0​: fri nov 5 23​:20​:39 pdt 2010; root​:xnu-1504.9.17~1release_i386 i386 '
  config_args='-de -DDEBUGGING -Duseithreads -Dusedevel -Accflags= -fcatch-undefined-behavior -DPERL_POISON'
  hint=recommended, useposix=true, d_sigaction=define
  useithreads=define, usemultiplicity=define
  useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
  use64bitint=undef, use64bitall=undef, uselongdouble=undef
  usemymalloc=n, bincompat5005=undef
  Compiler​:
  cc='gcc', ccflags ='-DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include',
  optimize='-O3 -g',
  cppflags='-DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
  ccversion='', gccversion='4.2.1 (Apple Inc. build 5664)', gccosandvers=''
  intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
  d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
  ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
  alignbytes=8, prototype=define
  Linker and Libraries​:
  ld='env MACOSX_DEPLOYMENT_TARGET=10.3 cc', ldflags =' -fstack-protector -L/usr/local/lib'
  libpth=/usr/local/lib /usr/lib
  libs=-ldbm -ldl -lm -lutil -lc
  perllibs=-ldl -lm -lutil -lc
  libc=, so=dylib, useshrplib=false, libperl=libperl.a
  gnulibc_version=''
  Dynamic Linking​:
  dlsrc=dl_dlopen.xs, dlext=bundle, d_dlsymun=undef, ccdlflags=' '
  cccdlflags=' ', lddlflags=' -bundle -undefined dynamic_lookup -L/usr/local/lib -fstack-protector'

Locally applied patches​:
 

---

@​INC for perl 5.17.6​:
  /usr/local/lib/perl5/site_perl/5.17.6/darwin-thread-multi-2level
  /usr/local/lib/perl5/site_perl/5.17.6
  /usr/local/lib/perl5/5.17.6/darwin-thread-multi-2level
  /usr/local/lib/perl5/5.17.6
  /usr/local/lib/perl5/site_perl
  .

---

Environment for perl 5.17.6​:
  DYLD_LIBRARY_PATH (unset)
  HOME=/Users/sprout
  LANG=en_US.UTF-8
  LANGUAGE (unset)
  LD_LIBRARY_PATH (unset)
  LOGDIR (unset)
  PATH=/usr/bin​:/bin​:/usr/sbin​:/sbin​:/usr/local/bin​:/usr/X11/bin​:/usr/local/bin
  PERL_BADLANG (unset)
  SHELL=/bin/bash

[p5pRT]

From zefram@fysh.org

Father Chrysostomos wrote​:

Is there any guarantee that a handle created like that will be accessible
like this?

Quite the contrary. perlfunc(1) describes the generated filehandle as
"anonymous", which would seem to preclude it being interned in the stash.
This description is applied to any case of an undef value being supplied;
there's no distinction corresponding to the difference between open($a)
and open(${\$a}).

For the glob to be sometimes interned is surprising
and undocumented. It is surely a bug. Fixed in commit
db9848c.

newGVgen, which creates a new handle (with a name like _GEN_0), also
adds it to the symbol table. It is an API function. Should it stop
adding it to the symbol table?

It's not documented, and interning isn't a crazy thing to do, so I'd
take the behaviour to be definitive. The bug isn't newGVgen() interning,
it's rv2gv using newGVgen().

But if most users of newGVgen() wouldn't be affected by it not interning,
that suggests that they don't actually want this behaviour, and so
newGVgen() is probably the wrong thing for them to use. Tickets should
be filed against the individual CPAN distros telling them to do something
else.

-zefram

[p5pRT]

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

[p5pRT]

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

[p5pRT]

From @cpansprout

On Dec 9, 2017, at 5​:52 PM, Zefram via RT <perlbug-followup@​perl.org> wrote​:

Father Chrysostomos wrote​:

newGVgen, which creates a new handle (with a name like _GEN_0), also
adds it to the symbol table. It is an API function. Should it stop
adding it to the symbol table?

It's not documented, and interning isn't a crazy thing to do, so I'd
take the behaviour to be definitive. The bug isn't newGVgen() interning,
it's rv2gv using newGVgen().

But if most users of newGVgen() wouldn't be affected by it not interning,
that suggests that they don't actually want this behaviour, and so
newGVgen() is probably the wrong thing for them to use. Tickets should
be filed against the individual CPAN distros telling them to do something
else.

The main offender is the default typemap. Please review the attached patch.

[p5pRT]

From @cpansprout

Inline Patch

diff --git a/lib/ExtUtils/typemap b/lib/ExtUtils/typemap
index 4bfba95..ca923cf 100644
--- a/lib/ExtUtils/typemap
+++ b/lib/ExtUtils/typemap
@@ -398,7 +398,8 @@ T_ARRAY
         }
 T_STDIO
 	{
-	    GV *gv = newGVgen("$Package");
+	    GV *gv = (GV *)sv_newmortal();
+	    gv_init_pvn(gv, gv_stashpvs("$Package",1),"__ANONIO__",10,0);
 	    PerlIO *fp = PerlIO_importFILE($var,0);
 	    if ( fp && do_open(gv, "+<&", 3, FALSE, 0, 0, fp) ) {
 		SV *rv = newRV_inc((SV*)gv);
@@ -411,7 +412,8 @@ T_STDIO
 	}
 T_IN
 	{
-	    GV *gv = newGVgen("$Package");
+	    GV *gv = (GV *)sv_newmortal();
+	    gv_init_pvn(gv, gv_stashpvs("$Package",1),"__ANONIO__",10,0);
 	    if ( do_open(gv, "<&", 2, FALSE, 0, 0, $var) ) {
 		SV *rv = newRV_inc((SV*)gv);
 		rv = sv_bless(rv, GvSTASH(gv));
@@ -423,7 +425,8 @@ T_IN
 	}
 T_INOUT
 	{
-	    GV *gv = newGVgen("$Package");
+	    GV *gv = (GV *)sv_newmortal();
+	    gv_init_pvn(gv, gv_stashpvs("$Package",1),"__ANONIO__",10,0);
 	    if ( do_open(gv, "+<&", 3, FALSE, 0, 0, $var) ) {
 		SV *rv = newRV_inc((SV*)gv);
 		rv = sv_bless(rv, GvSTASH(gv));
@@ -435,7 +438,8 @@ T_INOUT
 	}
 T_OUT
 	{
-	    GV *gv = newGVgen("$Package");
+	    GV *gv = (GV *)sv_newmortal();
+	    gv_init_pvn(gv, gv_stashpvs("$Package",1),"__ANONIO__",10,0);
 	    if ( do_open(gv, "+>&", 3, FALSE, 0, 0, $var) ) {
 		SV *rv = newRV_inc((SV*)gv);
 		rv = sv_bless(rv, GvSTASH(gv));

[p5pRT]

From zefram@fysh.org

Father Chrysostomos wrote​:

The main offender is the default typemap. Please review the attached patch.

Looks good to me. But there are way more of them needing the same change,
in other upstream=blead files.

-zefram

[p5pRT]

From @cpansprout

On Dec 10, 2017, at 3​:26 PM, "Zefram via RT" <perlbug-followup@​perl.org> wrote​:

Father Chrysostomos wrote​:

The main offender is the default typemap. Please review the attached patch.

Looks good to me. But there are way more of them needing the same change,
in other upstream=blead files.

Done in 6da090e and 732d389.

[p5pRT]

From zefram@fysh.org

Father Chrysostomos wrote​:

Done in 6da090e and 732d389.

You've broken something​:

APItest.c​: In function 'XS_XS__APItest_PerlIO_exportFILE'​:
APItest.c​:7295​:9​: warning​: assignment from incompatible pointer type
  RETVAL = PerlIO_exportFILE(f, mode);
  ^
APItest.c​:7302​:34​: warning​: passing argument 1 of 'PerlIO_importFILE' from incompatible pointer type
  PerlIO *fp = PerlIO_importFILE(RETVAL,0);
  ^
In file included from ../../iperlsys.h​:51​:0,
  from ../../perl.h​:3540,
  from APItest.xs​:10​:
../../perlio.h​:217​:23​: note​: expected 'struct FILE *' but argument is of type 'struct PerlIOl **'
PERL_CALLCONV PerlIO *PerlIO_importFILE(FILE *, const char *);
  ^
../../perlio.h​:63​:16​: error​: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
#define PerlIO PerlIO
  ^
APItest.c​:7302​:3​: note​: in expansion of macro 'PerlIO'
  PerlIO *fp = PerlIO_importFILE(RETVAL,0);
  ^
cc1​: some warnings being treated as errors
Makefile​:344​: recipe for target 'APItest.o' failed

-zefram

[p5pRT]

From @cpansprout

On Dec 10, 2017, at 7​:55 PM, "Zefram via RT" <perlbug-followup@​perl.org> wrote​:

Father Chrysostomos wrote​:

Done in 6da090e and 732d389.

You've broken something​:

APItest.c​: In function 'XS_XS__APItest_PerlIO_exportFILE'​:
APItest.c​:7295​:9​: warning​: assignment from incompatible pointer type
RETVAL = PerlIO_exportFILE(f, mode);
^

I added this to APItest.xs​:

FILE *
PerlIO_exportFILE(PerlIO *f, const char *mode)

The code generated in APItest.c is as follows​:

XS_EUPXS(XS_XS__APItest_PerlIO_exportFILE)
{
  dVAR; dXSARGS;
  if (items != 2)
  croak_xs_usage(cv, "f, mode");
  {
  FILE * RETVAL;
  PerlIO * f = IoIFP(sv_2io(ST(0)))
;
  const char * mode = (const char *)SvPV_nolen(ST(1))
;

  RETVAL = PerlIO_exportFILE(f, mode);
  {
  SV * RETVALSV;
  RETVALSV = sv_newmortal();
  {
  GV *gv = (GV *)sv_newmortal();
  gv_init_pvn(gv, gv_stashpvs("XS​::APItest",1),"__ANONIO__",10,0);
  PerlIO *fp = PerlIO_importFILE(RETVAL,0);
  if ( fp && do_open(gv, "+<&", 3, FALSE, 0, 0, fp) ) {
  SV *rv = newRV_inc((SV*)gv);
  rv = sv_bless(rv, GvSTASH(gv));
  RETVALSV = sv_2mortal(rv);
  }
  }
  ST(0) = RETVALSV;
  }
  }
  XSRETURN(1);
}

Notice that RETVAL is declared as a FILE *.

perlio.c declared the function thus​:

FILE *
PerlIO_exportFILE(PerlIO * f, const char *mode)

So how can that be an incompatible pointer type?

Maybe this has something to do with it​:

$ ./perl -Ilib Porting/expand-macro.pl FILE
cc -c -DPERL_CORE -fno-common -DPERL_DARWIN -mmacosx-version-min=10.8 -DDEBUGGING -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -I/opt/local/include -DPERL_USE_SAFE_PUTENV -std=c89 -O3 -O0 -g -Wall -Werror=declaration-after-statement -Werror=pointer-arith -Wextra -Wc++-compat -Wwrite-strings -Wno-unused-value -E try.c > try.i
# 4 "FILE expands to"
PerlIO

But where is that coming from?

‘ack -n 'define FILE'’ includes in its output​:

fakesdio.h
16​:#define FILE PerlIO

which means we have API functions (documented in perlapio) that are not callable without compiler warnings (or explicit casts).

APItest.c​:7302​:34​: warning​: passing argument 1 of 'PerlIO_importFILE' from incompatible pointer type
PerlIO *fp = PerlIO_importFILE(RETVAL,0);
^
In file included from ../../iperlsys.h​:51​:0,
from ../../perl.h​:3540,
from APItest.xs​:10​:
../../perlio.h​:217​:23​: note​: expected 'struct FILE *' but argument is of type 'struct PerlIOl **'
PERL_CALLCONV PerlIO *PerlIO_importFILE(FILE *, const char *);
^
../../perlio.h​:63​:16​: error​: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
#define PerlIO PerlIO
^
APItest.c​:7302​:3​: note​: in expansion of macro 'PerlIO'
PerlIO *fp = PerlIO_importFILE(RETVAL,0);
^

Well this one is my fault. It’s a good thing I added a test! Fixed in d269f58.

What are we supposed to do about ‘API’ functions like this? Or perhaps a more relevant question​: How do we test a FILE* typemap?

cc1​: some warnings being treated as errors
Makefile​:344​: recipe for target 'APItest.o' failed

-zefram

[p5pRT]

From @iabyn

On Sun, Dec 10, 2017 at 08​:27​:42PM -0800, Father Chrysostomos wrote​:

Well this one is my fault. It’s a good thing I added a test! Fixed in d269f58.

With v5.27.6-189-gd269f58, t/op/svleak.t is SEGVing on my system.
Reducing it to just the final test,

  leak 2,1,sub{XS​::APItest​::PerlIO_exportFILE(*STDIN,"");0},
  'T_STDIO in default typemap';

valgrind is giving me a whole bunch of​:

  ==21957== Invalid read of size 4
  ==21957== at 0x5C25FA0​: fileno (in /usr/lib64/libc-2.21.so)
  ==21957== by 0x78811E​: PerlIOStdio_fileno (perlio.c​:2885)
  ==21957== by 0x784D88​: Perl_PerlIO_fileno (perlio.c​:1391)
  ==21957== by 0x7893F3​: PerlIO_exportFILE (perlio.c​:3742)
  ==21957== by 0xD274D40​: XS_XS__APItest_PerlIO_exportFILE (APItest.c​:7295)
  ==21957== by 0x5FCB2B​: Perl_pp_entersub (pp_hot.c​:5241)
  ==21957== by 0x587203​: Perl_runops_debug (dump.c​:2495)
  ==21957== by 0x46DBB2​: S_run_body (perl.c​:2717)
  ==21957== by 0x46CF22​: perl_run (perl.c​:2633)
  ==21957== by 0x41C356​: main (perlmain.c​:123)
  ==21957== Address 0xd0a2730 is 0 bytes inside a block of size 552 free'd
  ==21957== at 0x4C29D6A​: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
  ==21957== by 0x5C1D9A2​: fclose@​@​GLIBC_2.2.5 (in /usr/lib64/libc-2.21.so)
  ==21957== by 0x788E04​: PerlIOStdio_close (perlio.c​:3314)
  ==21957== by 0x784C94​: PerlIO__close (perlio.c​:1365)
  ==21957== by 0x784CE0​: Perl_PerlIO_close (perlio.c​:1378)
  ==21957== by 0x720E89​: Perl_io_close (doio.c​:1523)
  ==21957== by 0x49261A​: Perl_gp_free (gv.c​:2676)
  ==21957== by 0x6399E5​: Perl_sv_clear (sv.c​:6765)
  ==21957== by 0x63C348​: Perl_sv_free2 (sv.c​:7136)
  ==21957== by 0x6A7791​: S_SvREFCNT_dec_NN (inline.h​:202)
  ==21957== by 0x6A8170​: Perl_free_tmps (scope.c​:216)
  ==21957== by 0x5DB002​: Perl_pp_nextstate (pp_hot.c​:52)

I haven't looked more closely.

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

[p5pRT]

From @cpansprout

On Dec 11, 2017, at 3​:54 AM, "Dave Mitchell via RT" <perlbug-followup@​perl.org> wrote​:

On Sun, Dec 10, 2017 at 08​:27​:42PM -0800, Father Chrysostomos wrote​:

Well this one is my fault. It’s a good thing I added a test! Fixed in d269f58.

With v5.27.6-189-gd269f58, t/op/svleak.t is SEGVing on my system.
Reducing it to just the final test,

leak 2,1,sub{XS​::APItest​::PerlIO_exportFILE(*STDIN,"");0},
'T_STDIO in default typemap';

valgrind is giving me a whole bunch of​:

Does the problem go away if you remove this line from APItest.xs​:

#include "fakesdio.h" /* Causes us to use PerlIO below */

Does the crash go away if you change

FILE *
PerlIO_exportFILE(PerlIO *f, const char *mode)

in APItest.xs to

FILE *
PerlIO_findFILE(PerlIO *f)

and change the test in svleak.t to​:

leak 2,1,sub{XS​::APItest​::PerlIO_findFILE(*STDIN);0},
  'T_STDIO in default typemap';

?

[p5pRT]

From @iabyn

On Mon, Dec 11, 2017 at 08​:25​:57AM -0800, Father Chrysostomos wrote​:

On Dec 11, 2017, at 3​:54 AM, "Dave Mitchell via RT" <perlbug-followup@​perl.org> wrote​:

On Sun, Dec 10, 2017 at 08​:27​:42PM -0800, Father Chrysostomos wrote​:

Well this one is my fault. It’s a good thing I added a test! Fixed in d269f58.

With v5.27.6-189-gd269f58, t/op/svleak.t is SEGVing on my system.
Reducing it to just the final test,

leak 2,1,sub{XS​::APItest​::PerlIO_exportFILE(*STDIN,"");0},
'T_STDIO in default typemap';

valgrind is giving me a whole bunch of​:

Does the problem go away if you remove this line from APItest.xs​:

#include "fakesdio.h" /* Causes us to use PerlIO below */

No.

Does the crash go away if you change

FILE *
PerlIO_exportFILE(PerlIO *f, const char *mode)

in APItest.xs to

FILE *
PerlIO_findFILE(PerlIO *f)

and change the test in svleak.t to​:

leak 2,1,sub{XS​::APItest​::PerlIO_findFILE(*STDIN);0},
'T_STDIO in default typemap';

No.

--
The crew of the Enterprise encounter an alien life form which is
surprisingly neither humanoid nor made from pure energy.
  -- Things That Never Happen in "Star Trek" #22

[p5pRT]

From @jkeenan

It appears that this ticket was incorrectly marked Resolved. Re-opening, as discussion continues.
--
James E Keenan (jkeenan@​cpan.org)