- 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 e3da02934700e859b9dd2e9595ed105fc52f8045..c33dd0a6ffd50b48862c10f834ff5657366c15b9 100755 (executable)
--- 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 c2a45af5d084cdc92f1aa7c77f1953d3a6d65ff8..1f0cc9edd1800c4583c7aa91d132abafca82fa39 100755 (executable)
--- 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 ce2e4056cf1aeee84412a39c97428947f7801384..b9eac0526128c3ea0028d858f532929a137b0843 100755 (executable)
--- 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 (executable)
index 0000000..6ed8035
--- /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 (executable)
index 0000000..00afa61
--- /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===