Front page | perl.perl5.porters |
Postings from November 2010
Re: [perl.git] branch blead, updated. v5.13.6-546-g53226d6
From:
Nicholas Clark
Date:
November 30, 2010 12:35
Subject:
Re: [perl.git] branch blead, updated. v5.13.6-546-g53226d6
Message ID:
20101130203519.GN24189@plum.flirble.org
On Sun, Nov 21, 2010 at 12:20:18PM -0800, Father Chrysostomos wrote:
> On November 18, 2010 01:48, Nicholas Clark wrote:
> > > + struct xpvhv_aux * const newaux = hv_auxinit(hv);
> >
> > ^^^^^^^^^
> > That leaks in some cases. I'm not sure what the correct fix should be. (Sorry)
>
> I?ve looked at it half a dozen times, and I cannot see how it is leaking. The auxiliary structure is allocated as part of the same memory block as HvARRAY. That gets freed either at the end of the loop body or, if the loop exits early, at the end of the function (hfreeentries).
>
> That commit *did* introduce a memory leak. It was leaking a HEK. But that was fixed a few commits later.
>
> There have been more changes since then. Does it leak in current blead?
>
> Does valgrind always tell the truth?
I can't think of an instance where it hasn't.
I think I can see the cause of the problem. A one line solution does not
present itself.
So, we come into the loop. All the conditions in the code below are true, this
time round, and HvARRAY(hv) is true, and points to a block of memory, with the
HV's linked lists in it, and the aux structure as the last part of that block.
The first line below is 1662. The assignment to `array` is 1666.
while (1) {
/* This is the one we're going to try to empty. First time round
it's the original array. (Hopefully there will only be 1 time
round) */
HE ** const array = HvARRAY(hv);
I32 i = HvMAX(hv);
struct xpvhv_aux *iter = SvOOK(hv) ? HvAUX(hv) : NULL;
/* make everyone else think the array is empty, so that the destructors
* called for freed entries can't recursively mess with us */
HvARRAY(hv) = NULL;
if (SvOOK(hv)) {
HE *entry;
SvFLAGS(hv) &= ~SVf_OOK; /* Goodbye, aux structure. */
/* What aux structure? */
/* (But we still have a pointer to it in iter.) */
/* Copy the name and MRO stuff to a new aux structure
if present. */
if (iter->xhv_name_u.xhvnameu_name || iter->xhv_mro_meta) {
struct xpvhv_aux * const newaux = hv_auxinit(hv);
at this point S_hv_auxinit() has just allocated a new hunk of memory for the
aux structure, which means a new block of memory, zeroed out, as there are
no linked lists this time. However, this is a new block of memory,
HvARRAY(hv) now points to it (no longer NULL), and the previous block of
memory is still around, pointed to by `array`.
We proceed to free the linked lists pointed to from `array`. We get to this
check, but HvARRAY(hv) is not NULL (it was assigned to in hv_auxinit())
So we don't break here:
if (!HvARRAY(hv)) {
/* Good. No-one added anything this time round. */
break;
}
and get to the end of the loop. And repeat.
Breakpoint 1, S_hfreeentries (hv=0x849f128) at hv.c:1800
1800 if (--attempts == 0) {
(gdb) n
1803 }
(gdb)
1666 HE ** const array = HvARRAY(hv);
(gdb)
1667 I32 i = HvMAX(hv);
(gdb)
1669 struct xpvhv_aux *iter = SvOOK(hv) ? HvAUX(hv) : NULL;
(gdb)
1673 HvARRAY(hv) = NULL;
(gdb)
1675 if (SvOOK(hv)) {
(gdb)
1678 SvFLAGS(hv) &= ~SVf_OOK; /* Goodbye, aux structure. */
(gdb)
1684 if (iter->xhv_name_u.xhvnameu_name || iter->xhv_mro_meta) {
(gdb)
1685 struct xpvhv_aux * const newaux = hv_auxinit(hv);
(gdb)
1686 newaux->xhv_name_count = iter->xhv_name_count;
(gdb)
1687 if (newaux->xhv_name_count)
(gdb)
1692 = iter->xhv_name_u.xhvnameu_name;
(gdb)
1691 newaux->xhv_name_u.xhvnameu_name
(gdb)
1692 = iter->xhv_name_u.xhvnameu_name;
(gdb)
1694 iter->xhv_name_u.xhvnameu_name = NULL;
(gdb)
1695 newaux->xhv_mro_meta = iter->xhv_mro_meta;
(gdb)
1696 iter->xhv_mro_meta = NULL;
(gdb)
1722 if (iter->xhv_backreferences) {
(gdb)
1746 entry = iter->xhv_eiter; /* HvEITER(hv) */
(gdb)
1747 if (entry && HvLAZYDEL(hv)) { /* was deleted earlier? */
(gdb)
1751 iter->xhv_riter = -1; /* HvRITER(hv) = -1 */
(gdb)
1752 iter->xhv_eiter = NULL; /* HvEITER(hv) = NULL */
(gdb)
1758 if (!((XPVHV*) SvANY(hv))->xhv_keys) break;
and break out of the loop.
`array` still points to allocated memory that needs freeing, allocated by the
first call to hv_auxint(). That memory leaks. HvARRAY(hv) (now) points to
memory allocated on the second trip round the loop, in the gdb steps shown
above.
And now we are out of the loop:
(gdb)
1806 if (SvOOK(hv)) current_aux = HvAUX(hv);
and we have leaked. The memory pointed to by HvARRAY(hv) on entry to
S_hfreeentries() has been freed. The second allocation in the loop is now
safely referenced by HvARRAY(hv). The first allocation in the loop is
unreferenced.
I don't know what the solution should be. The reason for the loop in the first
place was the problem that (if I understand it correctly) it is possible for
1: there still to be external references to this hv.
2: the destructor actions triggered by S_hfreenentries() to cause calls to
hv_store() on this hv.
in which case, HvARRAY() can become reallocated "underneath" us.
I wasn't expecting this loop itself to be the cause of the reallocations.
Nicholas Clark