Fix memory leak in hfreeentries

The change that made hfreeentries keep the name in place when iterat-
ing (2d0d1ecc) caused this statement at the end of the loop to be a
no-op for named hashes, because the HvARRAY is always present at the
end of the loop (it contains the name):

	if (!HvARRAY(hv)) {
	    /* Good. No-one added anything this time round.  */
	    break;
	}

So this line was added (by the same change) before the freeing of the
linked lists:

	/* If there are no keys, there is nothing left to free. */
	if (!((XPVHV*) SvANY(hv))->xhv_keys) break;

But that means that this, immediately after the freeing of the linked
lists and just before the if(!HvARRAY(hv)):

	if (array != orig_array) {
	    Safefree(array);
	}

was not being reached, resulting in a memory leak (that Nicholas
Clark found).

This is what would happen:

On entering hfreeentries, orig_array would be assigned the value
in HvARRAY.

   HvARRAY    = original array
   orig_array = original array

Then the main loop would be entered, which would assign
HvARRAY to array:

   HvARRAY    = original array
   orig_array = original array
   array      = original array

HvARRAY would be nulled out and assigned a new value by hv_auxinit:

   HvARRAY    = first new array
   orig_array = original array
   array      = original array

Then the loop would repeat:

   HvARRAY    = first new array
   orig_array = original array
   array      = first new array

Then the HvARRAY would once more be nulled and replaced via
hv_auxinit:

   HvARRAY    = second new array
   orig_array = original array
   array      = first new array

Then the if(no keys)break; statement would be reached, exit-
ing the loop:

   HvARRAY    = second new array
   orig_array = original array
   <nothing>  = first new array

So the first new array is never freed.

This commit skips the allocation of an extra array at the beginning of
the loop if there are no keys. Then it exits early at the same spot.

1 files changed, 25 insertions, 16 deletions

diff --git a/hv.c b/hv.c
index a9fedb4bfc..d0f452e6f9 100644
--- a/hv.c
+++ b/hv.c
@@ -1668,20 +1668,25 @@ S_hfreeentries(pTHX_ HV *hv)
 
 	struct xpvhv_aux *iter = SvOOK(hv) ? HvAUX(hv) : NULL;
 
+	/* If there are no keys, we only need to free items in the aux
+	   structure and then exit the loop. */
+	const bool empty = !((XPVHV*) SvANY(hv))->xhv_keys;
+
 	/* 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 (!empty) 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.) */
+	    if (!empty) {
+	      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) {
+	      /* 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);
 		newaux->xhv_name_count = iter->xhv_name_count;
 		if (newaux->xhv_name_count)
@@ -1694,13 +1699,13 @@ S_hfreeentries(pTHX_ HV *hv)
 		iter->xhv_name_u.xhvnameu_name = NULL;
 		newaux->xhv_mro_meta = iter->xhv_mro_meta;
 		iter->xhv_mro_meta = NULL;
-	    }
+	      }
 
-	    /* Because we have taken xhv_name and
-	       xhv_mro_meta out, the only allocated
-	       pointers in the aux structure that might exist are the back-
-	       reference array and xhv_eiter.
-	     */
+	      /* Because we have taken xhv_name and xhv_mro_meta out, the
+	         only allocated pointers in the aux structure that might
+	         exist are the back-reference array and xhv_eiter.
+	       */
+	    }
 
 	    /* weak references: if called from sv_clear(), the backrefs
 	     * should already have been killed; if there are any left, its
@@ -1751,11 +1756,12 @@ S_hfreeentries(pTHX_ HV *hv)
 	    iter->xhv_riter = -1; 	/* HvRITER(hv) = -1 */
 	    iter->xhv_eiter = NULL;	/* HvEITER(hv) = NULL */
 
-	    /* There are now no allocated pointers in the aux structure.  */
+	    /* There are now no allocated pointers in the aux structure
+	       unless the hash is empty. */
 	}
 
 	/* If there are no keys, there is nothing left to free. */
-	if (!((XPVHV*) SvANY(hv))->xhv_keys) break;
+	if (empty) break;
 
 	/* Since we have removed the HvARRAY (and possibly replaced it by
 	   calling hv_auxinit), set the number of keys accordingly. */
@@ -1801,10 +1807,13 @@ S_hfreeentries(pTHX_ HV *hv)
 	    Perl_die(aTHX_ "panic: hfreeentries failed to free hash - something is repeatedly re-creating entries");
 	}
     }
+
+    /* If the array was not replaced, the rest does not apply. */
+    if (HvARRAY(hv) == orig_array) return;
 	
     /* Set aside the current array for now, in case we still need it. */
     if (SvOOK(hv)) current_aux = HvAUX(hv);
-    if (HvARRAY(hv) && HvARRAY(hv) != orig_array)
+    if (HvARRAY(hv))
 	tmp_array = HvARRAY(hv);
 
     HvARRAY(hv) = orig_array;