develooper Front page | perl.perl5.porters | Postings from November 2010

Re: [perl.git] branch blead, updated. v5.13.6-546-g53226d6

Nicholas Clark
November 30, 2010 12:35
Re: [perl.git] branch blead, updated. v5.13.6-546-g53226d6
Message ID:
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.  */

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        }
1666            HE ** const array = HvARRAY(hv);
1667            I32 i = HvMAX(hv);
1669            struct xpvhv_aux *iter = SvOOK(hv) ? HvAUX(hv) : NULL;
1673            HvARRAY(hv) = NULL;
1675            if (SvOOK(hv)) {
1678                SvFLAGS(hv) &= ~SVf_OOK; /* Goodbye, aux structure.  */
1684                if (iter->xhv_name_u.xhvnameu_name || iter->xhv_mro_meta) {
1685                    struct xpvhv_aux * const newaux = hv_auxinit(hv);
1686                    newaux->xhv_name_count = iter->xhv_name_count;
1687                    if (newaux->xhv_name_count)
1692                            = iter->xhv_name_u.xhvnameu_name;
1691                        newaux->xhv_name_u.xhvnameu_name
1692                            = iter->xhv_name_u.xhvnameu_name;
1694                    iter->xhv_name_u.xhvnameu_name = NULL;
1695                    newaux->xhv_mro_meta = iter->xhv_mro_meta;
1696                    iter->xhv_mro_meta = NULL;
1722                if (iter->xhv_backreferences) {
1746                entry = iter->xhv_eiter; /* HvEITER(hv) */
1747                if (entry && HvLAZYDEL(hv)) {       /* was deleted earlier? */
1751                iter->xhv_riter = -1;       /* HvRITER(hv) = -1 */
1752                iter->xhv_eiter = NULL;     /* HvEITER(hv) = NULL */
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

And now we are out of the loop:

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

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 Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at | Group listing | About