Front page | perl.perl5.porters |
Postings from October 2003
Re: Fixed: [perl #24255] shared hash ref memory leak in 5.8.1
From:
Dave Mitchell
Date:
October 23, 2003 15:00
Subject:
Re: Fixed: [perl #24255] shared hash ref memory leak in 5.8.1
Message ID:
20031023220037.GD397@fdgroup.com
On Thu, Oct 23, 2003 at 10:30:20PM +0100, Nicholas Clark wrote:
> > Whether this will be integrated in 5.8.2 is up to Nick.
> Is it safe to add to maint?
> [I know that it's desirable to stem the leak, but is it unlikely to add
> new bugs?]
The 2nd half of the patch, the SAVETMPS stuff, should be completely
harmless - the worst it could do would be to slow down shared var assignment
infinitestimally, which given what overhead it already has, wouldn't be
noticed.
As for the setting AvREAL part of the patch, I have a slight lingering
doubt, simply because without that flag, shared AVs should have been
leaking all over the place, but they're clearly only ocasionally leaking.
So perhaps it gets set on other code paths. But I would feel more
comfortable if I understood it completely. Perhaps Arthur would know?
>
> > + if (SvTYPE(ssv) == SVt_PVAV) /* #24061 */
> > + AvREAL_on(ssv);
>
> Is AvREAL documented anywhere?
Don't be silly, this is Perl5 :-)
Actually, it's kind of documented in at the top of av.h. The idea is that
nearly all AVs have this flag set; it is in fact set by newAV (but *not*
set by sv_upgrade(a_null_sv, SVt_PVAV), which is the bug I was fixing.
When set, it means that all elements of the SV are refcounted, so for
example when clearing such an AV, each element within it would be
refcntdec'ed, and presumably freed. Typically, only a few AVs don't have
this flag set: the stack, padlists, and @_; it is assumed in these cases
that special-purpose code will do whatever's necessary. @_ is slighly
special; is has AvREAL off, but AvREIFY on. This means that certain types
of operation that would modify the array itself causes the array to be set
to AvREAL, and all its current elements to have their refcnt bumped up by
one. Thus in the normal case where @_ is just read and not modified, you
can get by without having initially update the refcnts on all its members.
But it's still refcount safe in the face of modification. (In theory
anyway.)
> > CALLER_CONTEXT;
> > }
> >
> > @@ -436,6 +438,12 @@
> > if (target) {
> > SV *tmp;
> > SHARED_CONTEXT;
> > + /* #24255: sv_setsv() (via sv_unref_flags()) may cause a
> > + * deferred free with sv_2mortal(). Ensure that the free_tmps
> > + * is done within this inpterpreter. DAPM.
> > + */
> > + ENTER;
> > + SAVETMPS;
> > tmp = newRV(SHAREDSvPTR(target));
> > sv_setsv_nomg(SHAREDSvPTR(shared), tmp);
> > SvREFCNT_dec(tmp);
> > @@ -444,6 +452,8 @@
> > SvOBJECT_on(SHAREDSvPTR(target));
> > SvSTASH(SHAREDSvPTR(target)) = (HV*)fake_stash;
> > }
> > + FREETMPS;
> > + LEAVE;
> > CALLER_CONTEXT;
> > }
> > else {
>
> This bit seems safe to me. (But who am I to judge?)
Yep, seems safe to me too. And you're to judge because you're the pumpkin:
you are supposed to have miraculously gained the wisdom of Solomon merely
by the acceptance of the role ;-)
--
Monto Blanco... scorchio!
- http://www.powerage.demon.co.uk/fastshow/chanel9.htm