Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

[perl #98092] Fix unreferenced scalar warnings in clone.t

Commit da6b625 triggered an unrelated bug, by rearranging things in
memory so that the conditions were right:

If @DB::args has been populated, and then the items in it have been
freed, they can end up being reused for any SV-ish things allocated
thereafter, even lexical pads.

Each CV (subroutine) has a list of pads, called the padlist, which is
the same structure as a Perl array (an AV) underneath.  The padlist’s
memory management is done in pad.c, as there are other things that
have to be done when its elements (the pads themselves) are freed.
So, to prevent av.c from trying to free those elements, the padlist is
not marked REAL; i.e., it’s marked as not having its elements refer-
ence-counted, even though they are: it’s just not handled in av.c

The ‘Attempt to free unreferenced scalar’ warnings emitted by
threads::shared’s clone.t occurred when padlists and pads ended up
using freed SVs that were still in @DB::args.  When a new thread was
created, the pads in @DB::args ended up getting cloned when @DB::args
was cloned; hence they were treated as non-reference-counting arrays,
and the pads inside them were cloned with a lower reference count than
they ought to have had (sv_dup was called, instead of sv_dup_inc).
The pad-duplication code (pad_dup), like the regular SV-duplication
code, checks first to see if a padlist has been cloned already, before
actually doing it.  There was also a problem with pad_dup not incre-
menting the reference count of an already-cloned padlist.

So this commit fixes the pad_dup reference-counting bug and also
leaves AvREAL on for padlists, until the very last moment when they
are freed (in pad_free) with SvREFCNT_dec.
  • Loading branch information...
commit 7d953ba8a0841912423af9b3e7d1daef726d5fb0 1 parent 840565a
Father Chrysostomos authored

Showing 1 changed file with 9 additions and 14 deletions. Show diff stats Hide diff stats

  1. +9 14 pad.c
23 pad.c
@@ -42,8 +42,9 @@ XSUBs don't have CvPADLIST set - dXSTARG fetches values from PL_curpad,
42 42 but that is really the callers pad (a slot of which is allocated by
43 43 every entersub).
44 44
45   -The CvPADLIST AV has does not have AvREAL set, so REFCNT of component items
46   -is managed "manual" (mostly in pad.c) rather than normal av.c rules.
  45 +The CvPADLIST AV has the REFCNT of its component items managed "manually"
  46 +(mostly in pad.c) rather than by normal av.c rules. So we mark it AvREAL
  47 +just before freeing it, to let av.c know not to touch the entries.
47 48 The items in the AV are not SVs as for a normal AV, but other AVs:
48 49
49 50 0'th Entry of the CvPADLIST is an AV which represents the "names" or rather
@@ -277,7 +278,6 @@ Perl_pad_new(pTHX_ int flags)
277 278 av_store(pad, 0, NULL);
278 279 }
279 280
280   - AvREAL_off(padlist);
281 281 /* Most subroutines never recurse, hence only need 2 entries in the padlist
282 282 array - names, and depth=1. The default for av_store() is to allocate
283 283 0..3, and even an explicit call to av_extend() with <3 will be rounded
@@ -444,6 +444,7 @@ Perl_cv_undef(pTHX_ CV *cv)
444 444 PL_comppad_name = NULL;
445 445 SvREFCNT_dec(sv);
446 446 }
  447 + AvREAL_off(CvPADLIST(cv));
447 448 SvREFCNT_dec(MUTABLE_SV(CvPADLIST(cv)));
448 449 CvPADLIST(cv) = NULL;
449 450 }
@@ -2132,15 +2133,9 @@ Perl_padlist_dup(pTHX_ AV *srcpad, CLONE_PARAMS *param)
2132 2133 if (!srcpad)
2133 2134 return NULL;
2134 2135
2135   - assert(!AvREAL(srcpad));
2136   -
2137 2136 if (param->flags & CLONEf_COPY_STACKS
2138 2137 || SvREFCNT(AvARRAY(srcpad)[1]) > 1) {
2139   - /* XXX padlists are real, but pretend to be not */
2140   - AvREAL_on(srcpad);
2141 2138 dstpad = av_dup_inc(srcpad, param);
2142   - AvREAL_off(srcpad);
2143   - AvREAL_off(dstpad);
2144 2139 assert (SvREFCNT(AvARRAY(srcpad)[1]) == 1);
2145 2140 } else {
2146 2141 /* CvDEPTH() on our subroutine will be set to 0, so there's no need
@@ -2154,17 +2149,17 @@ Perl_padlist_dup(pTHX_ AV *srcpad, CLONE_PARAMS *param)
2154 2149 SV **names;
2155 2150 SV **pad1a;
2156 2151 AV *args;
2157   - /* look for it in the table first.
2158   - I *think* that it shouldn't be possible to find it there.
2159   - Well, except for how Perl_sv_compile_2op() "works" :-( */
  2152 + /* Look for it in the table first, as the padlist may have ended up
  2153 + as an element of @DB::args (or theoretically even @_), so it may
  2154 + may have been cloned already. It may also be there because of
  2155 + how Perl_sv_compile_2op() "works". :-( */
2160 2156 dstpad = (AV*)ptr_table_fetch(PL_ptr_table, srcpad);
2161 2157
2162 2158 if (dstpad)
2163   - return dstpad;
  2159 + return (AV *)SvREFCNT_inc_simple_NN(dstpad);
2164 2160
2165 2161 dstpad = newAV();
2166 2162 ptr_table_store(PL_ptr_table, srcpad, dstpad);
2167   - AvREAL_off(dstpad);
2168 2163 av_extend(dstpad, 1);
2169 2164 AvARRAY(dstpad)[0] = MUTABLE_SV(av_dup_inc(AvARRAY(srcpad)[0], param));
2170 2165 names = AvARRAY(AvARRAY(dstpad)[0]);

0 comments on commit 7d953ba

Please sign in to comment.
Something went wrong with that request. Please try again.