From: Marcus Boerger <helly@php.net>
Date: Fri, 3 Mar 2006 21:35:16 +0000 (+0000)
Subject: - Fix SEGV with AppendIterator when base class constructor was not called
X-Git-Tag: RELEASE_1_2~15
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=7bb0ca9ce67ee0672bb7e8619424c91904fd457a;p=php

- Fix SEGV with AppendIterator when base class constructor was not called
- Generic check to prevent double call to constructors of SPL iterators
---

diff --git a/ext/spl/config.m4 b/ext/spl/config.m4
index e3da029347..c33dd0a6ff 100755
--- a/ext/spl/config.m4
+++ b/ext/spl/config.m4
@@ -18,15 +18,17 @@ int main(int argc, char **argv) {
 }
   ], [
     ac_result=1
-    AC_MSG_RESULT(yes)
   ],[
     ac_result=0
-    AC_MSG_RESULT(no)
   ], [
     ac_result=0
-    AC_MSG_RESULT(no)
   ])
   CPPFLAGS=$old_CPPFLAGS
+  if test "$ac_result"; then
+    AC_MSG_RESULT(yes)
+  else
+    AC_MSG_RESULT(no)
+  fi
   AC_DEFINE_UNQUOTED(HAVE_PACKED_OBJECT_VALUE, $ac_result, [Whether struct _zend_object_value is packed])
   AC_DEFINE(HAVE_SPL, 1, [Whether you want SPL (Standard PHP Library) support]) 
   PHP_NEW_EXTENSION(spl, php_spl.c spl_functions.c spl_engine.c spl_iterators.c spl_array.c spl_directory.c spl_sxe.c spl_exceptions.c spl_observer.c, $ext_shared)
diff --git a/ext/spl/spl_iterators.c b/ext/spl/spl_iterators.c
index c2a45af5d0..1f0cc9edd1 100755
--- a/ext/spl/spl_iterators.c
+++ b/ext/spl/spl_iterators.c
@@ -823,7 +823,7 @@ int spl_dual_it_call_method(char *method, INTERNAL_FUNCTION_PARAMETERS)
 		
 		success = SUCCESS;
 	} else {
-		php_error_docref(NULL TSRMLS_CC, E_ERROR, "Unable to call %s::%s()", intern->inner.ce->name, method);
+		php_error_docref(NULL TSRMLS_CC, E_ERROR, "Unable to call %v::%s()", intern->inner.ce->name, method);
 		success = FAILURE;
 	}
 
@@ -832,20 +832,33 @@ int spl_dual_it_call_method(char *method, INTERNAL_FUNCTION_PARAMETERS)
 }
 #endif
 
+#define SPL_CHECK_CTOR(intern, classname) \
+	if (intern->dit_type == DIT_Unknown) { \
+		zend_throw_exception_ex(spl_ce_BadMethodCallException, 0 TSRMLS_CC, "Classes derived from %v must call %v::__construct()", \
+				(spl_ce_##classname)->name, (spl_ce_##classname)->name); \
+		return; \
+	}
+
+#define APPENDIT_CHECK_CTOR(intern) SPL_CHECK_CTOR(intern, AppendIterator)
+
 static inline int spl_dual_it_fetch(spl_dual_it_object *intern, int check_more TSRMLS_DC);
 
-static inline spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, zend_class_entry *ce_inner, dual_it_type dit_type)
+inline spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, zend_class_entry *ce_base, zend_class_entry *ce_inner, dual_it_type dit_type)
 {
 	zval                 *zobject, *retval;
 	spl_dual_it_object   *intern;
 	zend_class_entry     *ce = NULL;
 	int                   inc_refcount = 1;
 
+	intern = (spl_dual_it_object*)zend_object_store_get_object(getThis() TSRMLS_CC);
+	
+	if (intern->dit_type != DIT_Unknown) {
+		zend_throw_exception_ex(spl_ce_BadMethodCallException, 0 TSRMLS_CC, "%v::getIterator() must be called exactly once per instance", ce_base->name);
+		return NULL;
+	}
 
 	php_set_error_handling(EH_THROW, spl_ce_InvalidArgumentException TSRMLS_CC);
 
-	intern = (spl_dual_it_object*)zend_object_store_get_object(getThis() TSRMLS_CC);
-
 	intern->dit_type = dit_type;
 	switch (dit_type) {
 		case DIT_LimitIterator: {
@@ -910,7 +923,7 @@ static inline spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAME
 				if (instanceof_function(ce, zend_ce_aggregate TSRMLS_CC)) {
 					zend_call_method_with_0_params(&zobject, ce, &ce->iterator_funcs.zf_new_iterator, "getiterator", &retval);
 					if (!retval || Z_TYPE_P(retval) != IS_OBJECT || !instanceof_function(Z_OBJCE_P(retval), zend_ce_traversable TSRMLS_CC)) {
-						zend_throw_exception_ex(spl_ce_LogicException, 0 TSRMLS_CC, "%s::getIterator() must return an object that implememnts Traversable", ce->name);
+						zend_throw_exception_ex(spl_ce_LogicException, 0 TSRMLS_CC, "%v::getIterator() must return an object that implememnts Traversable", ce->name);
 						php_set_error_handling(EH_NORMAL, NULL TSRMLS_CC);
 						return NULL;
 					}
@@ -968,9 +981,9 @@ static inline spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAME
 
 /* {{{ proto FilterIterator::__construct(Iterator it) 
    Create an Iterator from another iterator */
-SPL_METHOD(dual_it, __construct)
+SPL_METHOD(FilterIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_iterator, DIT_Default);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_FilterIterator, zend_ce_iterator, DIT_FilterIterator);
 } /* }}} */
 
 /* {{{ proto Iterator FilterIterator::getInnerIterator() 
@@ -1212,7 +1225,7 @@ SPL_METHOD(FilterIterator, next)
    Create a RecursiveFilterIterator from a RecursiveIterator */
 SPL_METHOD(RecursiveFilterIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RecursiveIterator, DIT_Default);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RecursiveFilterIterator, spl_ce_RecursiveIterator, DIT_RecursiveFilterIterator);
 } /* }}} */
 
 /* {{{ proto boolean RecursiveFilterIterator::hasChildren()
@@ -1246,7 +1259,7 @@ SPL_METHOD(RecursiveFilterIterator, getChildren)
    Create a ParentIterator from a RecursiveIterator */
 SPL_METHOD(ParentIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RecursiveIterator, DIT_Default);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_ParentIterator, spl_ce_RecursiveIterator, DIT_ParentIterator);
 } /* }}} */
 
 /* {{{ proto boolean ParentIterator::hasChildren()
@@ -1281,7 +1294,7 @@ SPL_METHOD(ParentIterator, getChildren)
    Create an RegExIterator from another iterator and a regular expression */
 SPL_METHOD(RegExIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_iterator, DIT_RegExIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RegExIterator, zend_ce_iterator, DIT_RegExIterator);
 } /* }}} */
 
 /* {{{ proto bool RegExIterator::accept()
@@ -1331,7 +1344,7 @@ SPL_METHOD(RegExIterator, accept)
    Create an RecursiveRegExIterator from another recursive iterator and a regular expression */
 SPL_METHOD(RecursiveRegExIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RecursiveIterator, DIT_RecursiveRegExIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RecursiveRegExIterator, spl_ce_RecursiveIterator, DIT_RecursiveRegExIterator);
 } /* }}} */
 #endif
 
@@ -1352,7 +1365,9 @@ static inline void spl_dual_it_free_storage(void *_object TSRMLS_DC)
 	
 	if (object->dit_type == DIT_AppendIterator) {
 		object->u.append.iterator->funcs->dtor(object->u.append.iterator TSRMLS_CC);
-		zval_ptr_dtor(&object->u.append.zarrayit);
+		if (object->u.append.zarrayit) {
+			zval_ptr_dtor(&object->u.append.zarrayit);
+		}
 	}
 
 	if (object->dit_type == DIT_CachingIterator || object->dit_type == DIT_RecursiveCachingIterator) {
@@ -1387,6 +1402,7 @@ static zend_object_value spl_dual_it_new(zend_class_entry *class_type TSRMLS_DC)
 	intern = emalloc(sizeof(spl_dual_it_object));
 	memset(intern, 0, sizeof(spl_dual_it_object));
 	intern->std.ce = class_type;
+	intern->dit_type = DIT_Unknown;
 
 	ALLOC_HASHTABLE(intern->std.properties);
 	zend_hash_init(intern->std.properties, 0, NULL, ZVAL_PTR_DTOR, 0);
@@ -1404,7 +1420,7 @@ ZEND_BEGIN_ARG_INFO(arginfo_filter_it___construct, 0)
 ZEND_END_ARG_INFO();
 
 static zend_function_entry spl_funcs_FilterIterator[] = {
-	SPL_ME(dual_it,         __construct,      arginfo_filter_it___construct, ZEND_ACC_PUBLIC)
+	SPL_ME(FilterIterator,  __construct,      arginfo_filter_it___construct, ZEND_ACC_PUBLIC)
 	SPL_ME(FilterIterator,  rewind,           NULL, ZEND_ACC_PUBLIC)
 	SPL_ME(dual_it,         valid,            NULL, ZEND_ACC_PUBLIC)
 	SPL_ME(dual_it,         key,              NULL, ZEND_ACC_PUBLIC)
@@ -1510,7 +1526,7 @@ static inline void spl_limit_it_seek(spl_dual_it_object *intern, long pos TSRMLS
    Construct a LimitIterator from an Iterator with a given starting offset and optionally a maximum count */
 SPL_METHOD(LimitIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_iterator, DIT_LimitIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_LimitIterator, zend_ce_iterator, DIT_LimitIterator);
 } /* }}} */
 
 /* {{{ proto void LimitIterator::rewind() 
@@ -1693,7 +1709,7 @@ static inline void spl_caching_it_rewind(spl_dual_it_object *intern TSRMLS_DC)
    Construct a CachingIterator from an Iterator */
 SPL_METHOD(CachingIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_iterator, DIT_CachingIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_CachingIterator, zend_ce_iterator, DIT_CachingIterator);
 } /* }}} */
 
 /* {{{ proto void CachingIterator::rewind()
@@ -1963,7 +1979,7 @@ static zend_function_entry spl_funcs_CachingIterator[] = {
    Create an iterator from a RecursiveIterator */
 SPL_METHOD(RecursiveCachingIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RecursiveIterator, DIT_RecursiveCachingIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_RecursiveCachingIterator, spl_ce_RecursiveIterator, DIT_RecursiveCachingIterator);
 } /* }}} */
 
 /* {{{ proto bolean RecursiveCachingIterator::hasChildren()
@@ -2009,7 +2025,7 @@ static zend_function_entry spl_funcs_RecursiveCachingIterator[] = {
    Create an iterator from anything that is traversable */
 SPL_METHOD(IteratorIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_traversable, DIT_IteratorIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_IteratorIterator, zend_ce_traversable, DIT_IteratorIterator);
 } /* }}} */
 
 static
@@ -2032,7 +2048,7 @@ static zend_function_entry spl_funcs_IteratorIterator[] = {
    Create an iterator from another iterator */
 SPL_METHOD(NoRewindIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_iterator, DIT_NoRewindIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_NoRewindIterator, zend_ce_iterator, DIT_NoRewindIterator);
 } /* }}} */
 
 /* {{{ proto void NoRewindIterator::rewind()
@@ -2124,7 +2140,7 @@ static zend_function_entry spl_funcs_NoRewindIterator[] = {
    Create an iterator from another iterator */
 SPL_METHOD(InfiniteIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_iterator, DIT_InfiniteIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_InfiniteIterator, zend_ce_iterator, DIT_InfiniteIterator);
 } /* }}} */
 
 /* {{{ proto InfiniteIterator::next()
@@ -2247,7 +2263,7 @@ void spl_append_it_next(spl_dual_it_object *intern TSRMLS_DC) /* {{{ */
    Create an AppendIterator */
 SPL_METHOD(AppendIterator, __construct)
 {
-	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, zend_ce_iterator, DIT_AppendIterator);
+	spl_dual_it_construct(INTERNAL_FUNCTION_PARAM_PASSTHRU, spl_ce_AppendIterator, zend_ce_iterator, DIT_AppendIterator);
 } /* }}} */
 
 /* {{{ proto void AppendIterator::append(Iterator it)
@@ -2258,6 +2274,8 @@ SPL_METHOD(AppendIterator, append)
 	zval *it;
 
 	intern = (spl_dual_it_object*)zend_object_store_get_object(getThis() TSRMLS_CC);
+	
+	APPENDIT_CHECK_CTOR(intern);
 
 	if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O", &it, zend_ce_iterator) == FAILURE) {
 		return;
@@ -2319,6 +2337,7 @@ SPL_METHOD(AppendIterator, getIteratorIndex)
 
 	intern = (spl_dual_it_object*)zend_object_store_get_object(getThis() TSRMLS_CC);
 
+	APPENDIT_CHECK_CTOR(intern);
 	spl_array_iterator_key(intern->u.append.zarrayit, return_value TSRMLS_CC);
 } /* }}} */
 
@@ -2330,6 +2349,7 @@ SPL_METHOD(AppendIterator, getArrayIterator)
 
 	intern = (spl_dual_it_object*)zend_object_store_get_object(getThis() TSRMLS_CC);
 
+	APPENDIT_CHECK_CTOR(intern);
 	RETURN_ZVAL(intern->u.append.zarrayit, 1, 0);
 } /* }}} */
 
diff --git a/ext/spl/spl_iterators.h b/ext/spl/spl_iterators.h
index ce2e4056cf..b9eac05261 100755
--- a/ext/spl/spl_iterators.h
+++ b/ext/spl/spl_iterators.h
@@ -59,6 +59,9 @@ PHP_FUNCTION(iterator_count);
 
 typedef enum {
 	DIT_Default = 0,
+	DIT_FilterIterator = DIT_Default,
+	DIT_RecursiveFilterIterator = DIT_Default,
+	DIT_ParentIterator = DIT_Default,
 	DIT_LimitIterator,
 	DIT_CachingIterator,
 	DIT_RecursiveCachingIterator,
@@ -70,6 +73,7 @@ typedef enum {
 	DIT_RegExIterator,
 	DIT_RecursiveRegExIterator,
 #endif
+	DIT_Unknown = ~0
 } dual_it_type;
 
 enum {
diff --git a/ext/spl/tests/iterator_030.phpt b/ext/spl/tests/iterator_030.phpt
new file mode 100755
index 0000000000..6ed8035ac1
--- /dev/null
+++ b/ext/spl/tests/iterator_030.phpt
@@ -0,0 +1,46 @@
+--TEST--
+SPL: EmptyIterator access
+--SKIPIF--
+<?php if (!extension_loaded("spl")) print "skip"; ?>
+--FILE--
+<?php
+
+$it = new EmptyIterator;
+
+var_dump($it->valid());
+$it->rewind();
+var_dump($it->valid());
+$it->next();
+var_dump($it->valid());
+
+try
+{
+	var_dump($it->key());
+}
+catch(BadMethodCallException $e)
+{
+	echo $e->getMessage() . "\n";
+}
+
+try
+{
+	var_dump($it->current());
+}
+catch(BadMethodCallException $e)
+{
+	echo $e->getMessage() . "\n";
+}
+
+var_dump($it->valid());
+
+?>
+===DONE===
+<?php exit(0); ?>
+--EXPECT--
+bool(false)
+bool(false)
+bool(false)
+Accessing the key of an EmptyIterator
+Accessing the value of an EmptyIterator
+bool(false)
+===DONE===
diff --git a/ext/spl/tests/iterator_031.phpt b/ext/spl/tests/iterator_031.phpt
new file mode 100755
index 0000000000..00afa61850
--- /dev/null
+++ b/ext/spl/tests/iterator_031.phpt
@@ -0,0 +1,118 @@
+--TEST--
+SPL: AppendIterator::append() rewinds when neccessary
+--SKIPIF--
+<?php if (!extension_loaded("spl")) print "skip"; ?>
+--FILE--
+<?php
+
+class MyArrayIterator extends ArrayIterator
+{
+	function rewind()
+	{
+		echo __METHOD__ . "\n";
+		parent::rewind();
+	}
+}
+
+$it = new MyArrayIterator(array(1,2));
+
+foreach($it as $k=>$v)
+{
+	echo "$k=>$v\n";
+}
+
+class MyAppendIterator extends AppendIterator
+{
+	function __construct()
+	{
+		echo __METHOD__ . "\n";
+	}
+
+	function rewind()
+	{
+		echo __METHOD__ . "\n";
+		parent::rewind();
+	}
+
+	function valid()
+	{
+		echo __METHOD__ . "\n";
+		return parent::valid();
+	}
+	
+	function append(Iterator $what)
+	{
+		echo __METHOD__ . "\n";
+		parent::append($what);
+	}
+	
+	function parent__construct()
+	{
+		parent::__construct();
+	}
+}
+
+$ap = new MyAppendIterator;
+
+try
+{
+	$ap->append($it);
+}
+catch(BadMethodCallException $e)
+{
+	echo $e->getMessage() . "\n";
+}
+
+$ap->parent__construct();
+
+try
+{
+	$ap->parent__construct($it);
+}
+catch(BadMethodCallException $e)
+{
+	echo $e->getMessage() . "\n";
+}
+
+$ap->append($it);
+$ap->append($it);
+$ap->append($it);
+
+foreach($ap as $k=>$v)
+{
+	echo "$k=>$v\n";
+}
+
+?>
+===DONE===
+<?php exit(0); ?>
+--EXPECT--
+MyArrayIterator::rewind
+0=>1
+1=>2
+MyAppendIterator::__construct
+MyAppendIterator::append
+Classes derived from AppendIterator must call AppendIterator::__construct()
+AppendIterator::getIterator() must be called exactly once per instance
+MyAppendIterator::append
+MyArrayIterator::rewind
+MyAppendIterator::append
+MyAppendIterator::append
+MyAppendIterator::rewind
+MyArrayIterator::rewind
+MyAppendIterator::valid
+0=>1
+MyAppendIterator::valid
+1=>2
+MyArrayIterator::rewind
+MyAppendIterator::valid
+0=>1
+MyAppendIterator::valid
+1=>2
+MyArrayIterator::rewind
+MyAppendIterator::valid
+0=>1
+MyAppendIterator::valid
+1=>2
+MyAppendIterator::valid
+===DONE===