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

[perl.git] branch blead, updated. v5.13.7-216-ga21fa3b

From:
Father Chrysostomos
Date:
November 30, 2010 16:27
Subject:
[perl.git] branch blead, updated. v5.13.7-216-ga21fa3b
Message ID:
E1PNaXb-0004Pu-E2@camel.ams6.corp.booking.com
In perl.git, the branch blead has been updated

<http://perl5.git.perl.org/perl.git/commitdiff/a21fa3b025feb558a9279178ad69b68bd03ace56?hp=833f1b9305adb63ba9ffb5b9b832e8008443dcf4>

- Log -----------------------------------------------------------------
commit a21fa3b025feb558a9279178ad69b68bd03ace56
Author: Jerry D. Hedden <jdhedden@cpan.org>
Date:   Tue Nov 30 18:22:35 2010 -0500

    Skip t/io/eintr.t on Cygwin, too - hangs

M	t/io/eintr.t

commit 4aa3a86749acdb989cb33981ae3dd1fde939b66a
Author: Father Chrysostomos <sprout@cpan.org>
Date:   Tue Nov 30 15:53:16 2010 -0800

    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.

M	hv.c
-----------------------------------------------------------------------

Summary of changes:
 hv.c         |   41 +++++++++++++++++++++++++----------------
 t/io/eintr.t |    2 +-
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/hv.c b/hv.c
index a9fedb4..d0f452e 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;
diff --git a/t/io/eintr.t b/t/io/eintr.t
index 73eddeb..a1d9966 100644
--- a/t/io/eintr.t
+++ b/t/io/eintr.t
@@ -43,7 +43,7 @@ if (exists $ENV{PERLIO} && $ENV{PERLIO} =~ /stdio/  ) {
 # on Win32, alarm() won't interrupt the read/write call.
 # Similar issues with VMS.
 
-if ($^O eq 'VMS' || $^O eq 'MSWin32') {
+if ($^O eq 'VMS' || $^O eq 'MSWin32' || $^O eq 'cygwin') {
 	skip_all('various portability issues');
 	exit 0;
 }

--
Perl5 Master Repository



nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About