develooper Front page | perl.perl5.porters | Postings from December 2008

[PATCH] sv_dup(): avoid cloning empty arrays

Thread Next
From:
kevin brintnall
Date:
December 24, 2008 22:21
Subject:
[PATCH] sv_dup(): avoid cloning empty arrays
Message ID:
1230173955-31516-1-git-send-email-kbrint@rufus.net
Testing the ARRAY pointer is insufficent.  Arrays emptied by 'shift' or
'pop' may still have non-NULL 'ARRAY' pointers.  Check more carefully to
determine whether the array has anything worth duplicating.  If not, reset
the FILL and MAX offsets to -1 just as 'undef @ARRAY' would.

This avoids potential corruption in the PL_ptr_table during perl_clone().

---

When an array is shift'ed until empty, its ARRAY pointer is advanced past
the end of the array.  The existing code sets items=0, and thus no entries
are copied.  The resulting malloc(0) should never be used to address an
object (cf. C99 7.20.3).  However, the new pointer from malloc(0) is
stored in the pointer table.

NB: When the array *exactly* fits in its malloc() allocation, and the
array is emptied with shift, the ARRAY pointer after shift MAY be a
pointer to another valid object.  If that object is later dup'ed using the
PL_ptr_table, it will not be properly duplicated.  Rather, the new object
will point to the malloc(0) region allocated during the incorrect ARRAY
dup.

---

The following test case illustrates the effect of the bug.  It dies
(SIBGUS) under FreeBSD 7 (-Dusethreads=y -Dusemymalloc=n).

	#!/usr/bin/perl
	use threads;
	shift @ARGV while @ARGV;  # simulate getopts()
	threads->create(sub {})->join;

On 32-bit versions (i386), it dies when there are exactly 12 arguments in
@ARGV.  On 64-bit versions (amd64), it dies when there are exactly 6
arguments.  In each case, @ARGV's private array is exactly 48 bytes.

The next malloc(48) is for $main::0's GV MAGIC* (see S_init_postdump_symbols).
FreeBSD 7 places it immediately after @ARGV's AvARRAY in memory.  When
mg_dup() attempts to dup this MAGIC*, it retrieves the invalid pointer
that was stored in PL_ptr_table.

In this particular case, the crash comes when the thread is joined.  The
malloc(0) region is minimally sized (2 bytes on FreeBSD 7), and so other
small malloc()s may follow it immediately in memory.  De-referencing this
pointer as a MAGIC* causes mg_free() to follow an invalid mg_moremagic
pointer, triggering SIGBUS.

---

I'm looking for a good way to write a test case for this problem.  I
didn't see any existing tests that examine the internal C memory
structures.  Suggestions welcome.

This patch is against blead.  Please consider back-porting this fix to 5.8
also.  I can provide a specific patch against maint-5.8 if necessary.

---
 sv.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/sv.c b/sv.c
index cfae3b7..09d3d76 100644
--- a/sv.c
+++ b/sv.c
@@ -10822,7 +10822,8 @@ Perl_sv_dup(pTHX_ const SV *const sstr, CLONE_PARAMS *const param)
 		IoBOTTOM_NAME(dstr)	= SAVEPV(IoBOTTOM_NAME(dstr));
 		break;
 	    case SVt_PVAV:
-		if (AvARRAY((const AV *)sstr)) {
+		/* avoid cloning an empty array */
+		if (AvARRAY((const AV *)sstr) && AvFILLp((const AV *)sstr) >= 0) {
 		    SV **dst_ary, **src_ary;
 		    SSize_t items = AvFILLp((const AV *)sstr) + 1;
 
@@ -10847,6 +10848,8 @@ Perl_sv_dup(pTHX_ const SV *const sstr, CLONE_PARAMS *const param)
 		else {
 		    AvARRAY(MUTABLE_AV(dstr))	= NULL;
 		    AvALLOC((const AV *)dstr)	= (SV**)NULL;
+		    AvMAX(  (const AV *)dstr)	= -1;
+		    AvFILLp((const AV *)dstr)	= -1;
 		}
 		break;
 	    case SVt_PVHV:
-- 
1.6.0.5


Thread Next


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