Skip to content

Commit c454d27

Browse files
jmikolajmikola
committed
PHPC-215: Fix Cursor iteration through IteratorIterator
This removes the invalidate_current handler. Due to SPL's implementation, our handler was being invoked between rewinding and checking if the current position was valid. Given that our validity check depends on inspecting a pointer to the current element (like MySQLi's result iterator), we cannot use invalidate_current. The current element must still be freed during iteration, so we will instead call a php_phongo_cursor_free_current() function as needed. Additionally, we should track a reference to the Cursor object zval on the iterator itself, which will ensure it is not garbage-collected during iteration (demonstrated in the segfault backtrace in PHPC-215). Lastly, this commit renames the Cursor object and iterator structs and related functions to be more consistent with implementions in PHP core.
1 parent e2e9d89 commit c454d27

File tree

6 files changed

+136
-57
lines changed

6 files changed

+136
-57
lines changed

‎php_phongo.c

Lines changed: 51 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1557,28 +1557,32 @@ void php_phongo_cursor_free(php_phongo_cursor_t *cursor)
15571557
}
15581558
}
15591559

1560-
/* {{{ Iterator */
1561-
static void phongo_cursor_iterator_invalidate_current(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
1560+
static void php_phongo_cursor_free_current(php_phongo_cursor_t *cursor) /* {{{ */
15621561
{
1563-
php_phongo_cursor_t *cursor = NULL;
1564-
1565-
cursor = ((phongo_cursor_it *)iter)->iterator.data;
15661562
if (cursor->visitor_data.zchild) {
15671563
zval_ptr_dtor(&cursor->visitor_data.zchild);
15681564
cursor->visitor_data.zchild = NULL;
15691565
}
15701566
} /* }}} */
15711567

1572-
static void phongo_cursor_iterator_dtor(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
1568+
/* {{{ Iterator */
1569+
static void php_phongo_cursor_iterator_dtor(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
15731570
{
1574-
efree(iter);
1571+
php_phongo_cursor_iterator *cursor_it = (php_phongo_cursor_iterator *)iter;
1572+
1573+
if (cursor_it->intern.data) {
1574+
zval_ptr_dtor((zval**)&cursor_it->intern.data);
1575+
cursor_it->intern.data = NULL;
1576+
}
1577+
1578+
php_phongo_cursor_free_current(cursor_it->cursor);
1579+
1580+
efree(cursor_it);
15751581
} /* }}} */
15761582

1577-
static int phongo_cursor_iterator_valid(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
1583+
static int php_phongo_cursor_iterator_valid(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
15781584
{
1579-
php_phongo_cursor_t *cursor = NULL;
1580-
1581-
cursor = ((phongo_cursor_it *)iter)->iterator.data;
1585+
php_phongo_cursor_t *cursor = ((php_phongo_cursor_iterator *)iter)->cursor;
15821586

15831587
if (cursor->visitor_data.zchild) {
15841588
return SUCCESS;
@@ -1588,37 +1592,34 @@ static int phongo_cursor_iterator_valid(zend_object_iterator *iter TSRMLS_DC) /*
15881592
} /* }}} */
15891593

15901594
#if PHP_VERSION_ID < 50500
1591-
static int phongo_cursor_iterator_get_current_key(zend_object_iterator *iter, char **str_key, uint *str_key_len, ulong *int_key TSRMLS_DC) /* {{{ */
1595+
static int php_phongo_cursor_iterator_get_current_key(zend_object_iterator *iter, char **str_key, uint *str_key_len, ulong *int_key TSRMLS_DC) /* {{{ */
15921596
{
1593-
*int_key = (ulong) ((phongo_cursor_it *)iter)->current;
1597+
*int_key = (ulong) ((php_phongo_cursor_iterator *)iter)->current;
15941598
return HASH_KEY_IS_LONG;
15951599
} /* }}} */
15961600
#else
1597-
static void phongo_cursor_iterator_get_current_key(zend_object_iterator *iter, zval *key TSRMLS_DC) /* {{{ */
1601+
static void php_phongo_cursor_iterator_get_current_key(zend_object_iterator *iter, zval *key TSRMLS_DC) /* {{{ */
15981602
{
1599-
ZVAL_LONG(key, ((phongo_cursor_it *)iter)->current);
1603+
ZVAL_LONG(key, ((php_phongo_cursor_iterator *)iter)->current);
16001604
} /* }}} */
16011605
#endif
16021606

1603-
static void phongo_cursor_iterator_get_current_data(zend_object_iterator *iter, zval ***data TSRMLS_DC) /* {{{ */
1607+
static void php_phongo_cursor_iterator_get_current_data(zend_object_iterator *iter, zval ***data TSRMLS_DC) /* {{{ */
16041608
{
1605-
php_phongo_cursor_t *cursor = NULL;
1606-
1607-
cursor = ((phongo_cursor_it *)iter)->iterator.data;
1609+
php_phongo_cursor_t *cursor = ((php_phongo_cursor_iterator *)iter)->cursor;
16081610

16091611
*data = &cursor->visitor_data.zchild;
16101612
} /* }}} */
16111613

1612-
static void phongo_cursor_iterator_move_forward(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
1614+
static void php_phongo_cursor_iterator_move_forward(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
16131615
{
1614-
php_phongo_cursor_t *cursor = NULL;
1615-
phongo_cursor_it *cursor_it = (phongo_cursor_it *)iter;
1616-
const bson_t *doc;
1616+
php_phongo_cursor_iterator *cursor_it = (php_phongo_cursor_iterator *)iter;
1617+
php_phongo_cursor_t *cursor = cursor_it->cursor;
1618+
const bson_t *doc;
16171619

1618-
cursor = ((phongo_cursor_it *)iter)->iterator.data;
1619-
iter->funcs->invalidate_current(iter TSRMLS_CC);
1620+
php_phongo_cursor_free_current(cursor);
1621+
cursor_it->current++;
16201622

1621-
((phongo_cursor_it *)iter)->current++;
16221623
if (bson_iter_next(&cursor_it->first_batch_iter)) {
16231624
if (BSON_ITER_HOLDS_DOCUMENT (&cursor_it->first_batch_iter)) {
16241625
const uint8_t *data = NULL;
@@ -1635,20 +1636,18 @@ static void phongo_cursor_iterator_move_forward(zend_object_iterator *iter TSRML
16351636
MAKE_STD_ZVAL(cursor->visitor_data.zchild);
16361637
bson_to_zval(bson_get_data(doc), doc->len, &cursor->visitor_data);
16371638
} else {
1638-
iter->funcs->invalidate_current(iter TSRMLS_CC);
1639+
/* TODO: is this really necessary? */
1640+
php_phongo_cursor_free_current(cursor);
16391641
}
1640-
16411642
} /* }}} */
16421643

1643-
static void phongo_cursor_iterator_rewind(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
1644+
static void php_phongo_cursor_iterator_rewind(zend_object_iterator *iter TSRMLS_DC) /* {{{ */
16441645
{
1645-
php_phongo_cursor_t *cursor = NULL;
1646-
phongo_cursor_it *cursor_it = (phongo_cursor_it *)iter;
1646+
php_phongo_cursor_iterator *cursor_it = (php_phongo_cursor_iterator *)iter;
1647+
php_phongo_cursor_t *cursor = cursor_it->cursor;
16471648

1648-
cursor = ((phongo_cursor_it *)iter)->iterator.data;
1649-
1650-
iter->funcs->invalidate_current(iter TSRMLS_CC);
1651-
((phongo_cursor_it *)iter)->current = 0;
1649+
php_phongo_cursor_free_current(cursor);
1650+
cursor_it->current = 0;
16521651

16531652
/* firstBatch is empty when the query simply didn't return any results */
16541653
if (cursor->firstBatch) {
@@ -1674,34 +1673,33 @@ static void phongo_cursor_iterator_rewind(zend_object_iterator *iter TSRMLS_DC)
16741673
} /* }}} */
16751674

16761675
/* iterator handler table */
1677-
zend_object_iterator_funcs phongo_cursor_iterator_funcs = {
1678-
phongo_cursor_iterator_dtor,
1679-
phongo_cursor_iterator_valid,
1680-
phongo_cursor_iterator_get_current_data,
1681-
phongo_cursor_iterator_get_current_key,
1682-
phongo_cursor_iterator_move_forward,
1683-
phongo_cursor_iterator_rewind,
1684-
phongo_cursor_iterator_invalidate_current
1676+
zend_object_iterator_funcs php_phongo_cursor_iterator_funcs = {
1677+
php_phongo_cursor_iterator_dtor,
1678+
php_phongo_cursor_iterator_valid,
1679+
php_phongo_cursor_iterator_get_current_data,
1680+
php_phongo_cursor_iterator_get_current_key,
1681+
php_phongo_cursor_iterator_move_forward,
1682+
php_phongo_cursor_iterator_rewind,
1683+
NULL /* invalidate_current is not used */
16851684
};
16861685

1687-
zend_object_iterator *phongo_cursor_get_iterator(zend_class_entry *ce, zval *object, int by_ref TSRMLS_DC) /* {{{ */
1686+
zend_object_iterator *php_phongo_cursor_get_iterator(zend_class_entry *ce, zval *object, int by_ref TSRMLS_DC) /* {{{ */
16881687
{
1689-
php_phongo_cursor_t *cursor = (php_phongo_cursor_t *)zend_object_store_get_object(object TSRMLS_CC);
1690-
phongo_cursor_it *cursor_it = NULL;
1688+
php_phongo_cursor_iterator *cursor_it = NULL;
16911689

16921690
if (by_ref) {
16931691
zend_error(E_ERROR, "An iterator cannot be used with foreach by reference");
16941692
}
16951693

1696-
cursor_it = ecalloc(1, sizeof(phongo_cursor_it));
1694+
cursor_it = ecalloc(1, sizeof(php_phongo_cursor_iterator));
16971695

1698-
if (cursor->visitor_data.zchild) {
1699-
zval_ptr_dtor(&cursor->visitor_data.zchild);
1700-
cursor->visitor_data.zchild = NULL;
1701-
}
1696+
Z_ADDREF_P(object);
1697+
cursor_it->intern.data = (void*)object;
1698+
cursor_it->intern.funcs = &php_phongo_cursor_iterator_funcs;
1699+
cursor_it->cursor = (php_phongo_cursor_t *)zend_object_store_get_object(object TSRMLS_CC);
1700+
/* cursor_it->current should already be allocated to zero */
17021701

1703-
cursor_it->iterator.data = cursor;
1704-
cursor_it->iterator.funcs = &phongo_cursor_iterator_funcs;
1702+
php_phongo_cursor_free_current(cursor_it->cursor);
17051703

17061704
return (zend_object_iterator*)cursor_it;
17071705
} /* }}} */

‎php_phongo.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,6 @@ int phongo_execute_single_update(mongoc_client_t *client, c
115115
int phongo_execute_single_delete(mongoc_client_t *client, const char *namespace, const bson_t *query, const mongoc_write_concern_t *write_concern, int server_id, mongoc_delete_flags_t flags, zval *return_value, int return_value_used TSRMLS_DC);
116116

117117
mongoc_stream_t* phongo_stream_initiator (const mongoc_uri_t *uri, const mongoc_host_list_t *host, void *user_data, bson_error_t *error);
118-
zend_object_iterator* phongo_cursor_get_iterator (zend_class_entry *ce, zval *object, int by_ref TSRMLS_DC);
119118
const mongoc_read_prefs_t* phongo_read_preference_from_zval(zval *zread_preference TSRMLS_DC);
120119
const mongoc_write_concern_t* phongo_write_concern_from_zval (zval *zwrite_concern TSRMLS_DC);
121120
const php_phongo_query_t* phongo_query_from_zval (zval *zquery TSRMLS_DC);
@@ -142,6 +141,7 @@ zend_bool phongo_writeerror_init(zval *return_value, bson_t *bson TSRMLS_DC);
142141
zend_bool phongo_writeconcernerror_init(zval *return_value, bson_t *bson TSRMLS_DC);
143142

144143
void php_phongo_cursor_free(php_phongo_cursor_t *cursor);
144+
zend_object_iterator* php_phongo_cursor_get_iterator(zend_class_entry *ce, zval *object, int by_ref TSRMLS_DC);
145145

146146
#define PHONGO_CE_INIT(ce) do { \
147147
ce->ce_flags |= ZEND_ACC_FINAL_CLASS; \

‎php_phongo_classes.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,11 @@ typedef struct {
5757
} php_phongo_cursor_t;
5858

5959
typedef struct {
60-
zend_object_iterator iterator;
60+
zend_object_iterator intern;
6161
bson_iter_t first_batch_iter;
62+
php_phongo_cursor_t *cursor;
6263
long current;
63-
} phongo_cursor_it;
64+
} php_phongo_cursor_iterator;
6465

6566
typedef struct {
6667
zend_object std;

‎src/MongoDB/Cursor.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ PHP_MINIT_FUNCTION(Cursor)
276276
php_phongo_cursor_ce = zend_register_internal_class(&ce TSRMLS_CC);
277277
php_phongo_cursor_ce->create_object = php_phongo_cursor_create_object;
278278
PHONGO_CE_INIT(php_phongo_cursor_ce);
279-
php_phongo_cursor_ce->get_iterator = phongo_cursor_get_iterator;
279+
php_phongo_cursor_ce->get_iterator = php_phongo_cursor_get_iterator;
280280

281281
memcpy(&php_phongo_handler_cursor, phongo_get_std_object_handlers(), sizeof(zend_object_handlers));
282282
php_phongo_handler_cursor.get_debug_info = php_phongo_cursor_get_debug_info;

‎tests/standalone/cursor-IteratorIterator-001.phpt

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
--TEST--
2+
MongoDB\Driver\Cursor query result iteration through IteratorIterator
3+
--SKIPIF--
4+
<?php require "tests/utils/basic-skipif.inc"; CLEANUP(STANDALONE) ?>
5+
--FILE--
6+
<?php
7+
require_once "tests/utils/basic.inc";
8+
9+
$manager = new MongoDB\Driver\Manager(STANDALONE);
10+
11+
$manager->executeInsert(NS, array('_id' => 1, 'x' => 1));
12+
$manager->executeInsert(NS, array('_id' => 2, 'x' => 1));
13+
14+
$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query(array("x" => 1)));
15+
16+
foreach (new IteratorIterator($cursor) as $document) {
17+
var_dump($document);
18+
}
19+
20+
?>
21+
===DONE===
22+
<?php exit(0); ?>
23+
--EXPECT--
24+
array(2) {
25+
["_id"]=>
26+
int(1)
27+
["x"]=>
28+
int(1)
29+
}
30+
array(2) {
31+
["_id"]=>
32+
int(2)
33+
["x"]=>
34+
int(1)
35+
}
36+
===DONE===

‎tests/standalone/cursor-IteratorIterator-002.phpt

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
--TEST--
2+
MongoDB\Driver\Cursor command result iteration through IteratorIterator
3+
--SKIPIF--
4+
<?php require "tests/utils/basic-skipif.inc"; CLEANUP(STANDALONE) ?>
5+
--FILE--
6+
<?php
7+
require_once "tests/utils/basic.inc";
8+
9+
$manager = new MongoDB\Driver\Manager(STANDALONE);
10+
11+
$manager->executeInsert(NS, array('_id' => 1, 'x' => 1));
12+
$manager->executeInsert(NS, array('_id' => 2, 'x' => 1));
13+
14+
$command = new MongoDB\Driver\Command(array(
15+
'aggregate' => COLLECTION_NAME,
16+
'pipeline' => array(
17+
array('$match' => array('x' => 1)),
18+
),
19+
'cursor' => new stdClass,
20+
));
21+
22+
$cursor = $manager->executeCommand(DATABASE_NAME, $command);
23+
24+
foreach (new IteratorIterator($cursor) as $document) {
25+
var_dump($document);
26+
}
27+
28+
?>
29+
===DONE===
30+
<?php exit(0); ?>
31+
--EXPECT--
32+
array(2) {
33+
["_id"]=>
34+
int(1)
35+
["x"]=>
36+
int(1)
37+
}
38+
array(2) {
39+
["_id"]=>
40+
int(2)
41+
["x"]=>
42+
int(1)
43+
}
44+
===DONE===

0 commit comments

Comments
 (0)