Bug #76296 for DBI: _install_method fails to set CvFILE correctly

Tue Apr 03 23:46:36 2012 $_ = 'spro^^*%*^6ut# [...] &$%*c>#!^!#&!pan.org'; y/a-z.@//cd; print - Ticket created

Subject:

_install_method fails to set CvFILE correctly

$ perl5.15.9 -MDBI -MDevel::Peek -le 'Dump +DBI::db->can("FETCH")' SV = IV(0x834d0c) at 0x834d10 REFCNT = 1 FLAGS = (TEMP,ROK) RV = 0x89b590 SV = PVCV(0x8a9ec0) at 0x89b590 REFCNT = 3 FLAGS = (ISXSUB) COMP_STASH = 0x0 XSUB = 0x1bc110 XSUBANY = 2995200 GVGV::GV = 0x89b580 "DBI::common" :: "FETCH" FILE = "pan/build/namespace-clean-0.23-sEAgdO/blib/lib/Devel/Peek.pmc" DEPTH = 0 FLAGS = 0x8 OUTSIDE_SEQ = 0 PADLIST = 0x0 OUTSIDE = 0x0 (null) Notice that strange FILE string. The documentation for newXS says: I<filename> needs to be static storage, as it is used directly as CvFILE(), without a copy being made. But DBI is using the pv from inside the SV passed to _install_method. So CvFILE ends up pointing to freed memory, which may get reused by the time CvFILE is looked at. In perl 5.8.8 and earlier, the only way to fix this is to leak the file name, which is exactly what perl itself did for constant subs. perl 5.8.1 has this on line 4436 of op.c: cv = newXS(name, const_sv_xsub, savepv(CopFILE(PL_curcop))); Starting with 5.10.0 and 5.8.9, there is an undocumented newXS_flags function, used pretty much everywhere (so there’s little chance it will change), whose signature is: CV * Perl_newXS_flags(pTHX_ const char *name, XSUBADDR_t subaddr, const char *const filename, const char *const proto, U32 flags) One of the flags it accepts is XS_DYNAMIC_FILENAME, which tells it to copy the filename passed to it, and free it at the same time the CV itself is freed. As currently written, _install_method might as well pass NULL for the filename, because its not accomplishing anything by passing a scalar about to be freed.

Wed Apr 04 00:34:15 2012 $_ = 'spro^^*%*^6ut# [...] &$%*c>#!^!#&!pan.org'; y/a-z.@//cd; print - Correspondence added

On Tue Apr 03 23:46:36 2012, SPROUT wrote: Show quoted text

> Starting with 5.10.0 and 5.8.9, there is an undocumented newXS_flags > function, used pretty > much everywhere (so there’s little chance it will change), whose > signature is: > > CV * > Perl_newXS_flags(pTHX_ const char *name, XSUBADDR_t subaddr, > const char *const filename, const char *const proto, > U32 flags) > > One of the flags it accepts is XS_DYNAMIC_FILENAME, which tells it to > copy the filename > passed to it, and free it at the same time the CV itself is freed.

Alternatively, if you want a method that works in all versions, you can set SvPVX and SvLEN: file = savepv(file); cv = newXS(meth_name, XS_DBI_dispatch, file); SvPVX(cv) = file; SvLEN(cv) = 1; /* so that SvPVX is freed */ If such an XSUB is used as an AUTOLOAD sub, the file name will leak at that point (XS_DYNAMIC_FILENAME itself has the same problem in 5.14 and earlier), but you can solve that by checking in XS_DBI_dispatch whether SvPVX(cv) == CvFILE(cv) and setting SvPVX/SvLEN again.

Wed Apr 04 00:34:16 2012 The RT System itself - Status changed from 'new' to 'open'

Wed Apr 04 00:35:34 2012 $_ = 'spro^^*%*^6ut# [...] &$%*c>#!^!#&!pan.org'; y/a-z.@//cd; print - Correspondence added

On Wed Apr 04 00:34:15 2012, SPROUT wrote: Show quoted text

> On Tue Apr 03 23:46:36 2012, SPROUT wrote:

> > Starting with 5.10.0 and 5.8.9, there is an undocumented newXS_flags > > function, used pretty > > much everywhere (so there’s little chance it will change), whose > > signature is: > > > > CV * > > Perl_newXS_flags(pTHX_ const char *name, XSUBADDR_t subaddr, > > const char *const filename, const char *const

> proto,

> > U32 flags) > > > > One of the flags it accepts is XS_DYNAMIC_FILENAME, which tells it

> to

> > copy the filename > > passed to it, and free it at the same time the CV itself is freed.

> > Alternatively, if you want a method that works in all versions, you > can set SvPVX and SvLEN: > > file = savepv(file); > cv = newXS(meth_name, XS_DBI_dispatch, file); > SvPVX(cv) = file; > SvLEN(cv) = 1; /* so that SvPVX is freed */

To avoid compiler warnings, those should be SvPVX((SV *)cv) and SvLEN((SV *)cv). Show quoted text

> > If such an XSUB is used as an AUTOLOAD sub, the file name will leak at > that point > (XS_DYNAMIC_FILENAME itself has the same problem in 5.14 and earlier), > but you can solve > that by checking in XS_DBI_dispatch whether SvPVX(cv) == CvFILE(cv) > and setting > SvPVX/SvLEN again.

Wed Apr 04 00:39:02 2012 $_ = 'spro^^*%*^6ut# [...] &$%*c>#!^!#&!pan.org'; y/a-z.@//cd; print - Correspondence added

On Wed Apr 04 00:35:34 2012, SPROUT wrote: Show quoted text

> On Wed Apr 04 00:34:15 2012, SPROUT wrote:

> > On Tue Apr 03 23:46:36 2012, SPROUT wrote:

> > > Starting with 5.10.0 and 5.8.9, there is an undocumented

> newXS_flags

> > > function, used pretty > > > much everywhere (so there’s little chance it will change), whose > > > signature is: > > > > > > CV * > > > Perl_newXS_flags(pTHX_ const char *name, XSUBADDR_t subaddr, > > > const char *const filename, const char *const

> > proto,

> > > U32 flags) > > > > > > One of the flags it accepts is XS_DYNAMIC_FILENAME, which tells it

> > to

> > > copy the filename > > > passed to it, and free it at the same time the CV itself is freed.

> > > > Alternatively, if you want a method that works in all versions, you > > can set SvPVX and SvLEN: > > > > file = savepv(file); > > cv = newXS(meth_name, XS_DBI_dispatch, file); > > SvPVX(cv) = file; > > SvLEN(cv) = 1; /* so that SvPVX is freed */

> > To avoid compiler warnings, those should be SvPVX((SV *)cv) and > SvLEN((SV *)cv).

Attached is that suggestion as a patch. Show quoted text

>

> > > > If such an XSUB is used as an AUTOLOAD sub, the file name will leak

> at

> > that point > > (XS_DYNAMIC_FILENAME itself has the same problem in 5.14 and

> earlier),

> > but you can solve > > that by checking in XS_DBI_dispatch whether SvPVX(cv) == CvFILE(cv) > > and setting > > SvPVX/SvLEN again.

These aren’t ever used for AUTOLOAD, are they?

Subject:

open_qqPvVqas.txt

diff -rup DBI-1.618-aha5Y2/DBI.xs DBI-1.618-aha5Y2-copy/DBI.xs --- DBI-1.618-aha5Y2/DBI.xs 2012-04-03 21:37:46.000000000 -0700 +++ DBI-1.618-aha5Y2-copy/DBI.xs 2012-04-03 21:35:50.000000000 -0700 @@ -4573,7 +4573,10 @@ _install_method(dbi_class, meth_name, fi } if (trace_msg) PerlIO_printf(DBILOGFP,"%s\n", SvPV_nolen(trace_msg)); + file = savepv(file); cv = newXS(meth_name, XS_DBI_dispatch, file); + SvPVX((SV *)cv) = file; + SvLEN((SV *)cv) = 1; CvXSUBANY(cv).any_ptr = ima; ST(0) = &PL_sv_yes; }

Thu Apr 19 04:04:57 2012 bohica [...] ntlworld.com - Correspondence added

On Wed Apr 04 00:39:02 2012, SPROUT wrote: Show quoted text

> Attached is that suggestion as a patch.

Thanks, applied to subversion trunk and should be in next release. Martin -- Martin J. Evans Wetherby, UK

Thu Apr 19 04:05:42 2012 bohica [...] ntlworld.com - Status changed from 'open' to 'patched'

Wed Apr 25 04:07:35 2012 bohica [...] ntlworld.com - Status changed from 'patched' to 'resolved'

Wed Apr 25 04:07:36 2012 bohica [...] ntlworld.com - Fixed in 1.619 added