Skip to content

Commit c08f056

Browse files
jmikolajmikola
committed
Merge pull request #613 
2 parents dac9df6 + e089c2b commit c08f056

File tree

5 files changed

+127
-20
lines changed

5 files changed

+127
-20
lines changed

‎php_phongo.c‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1431,7 +1431,7 @@ static void php_phongo_dispatch_handlers(const char *name, zval *z_event)
14311431
#if PHP_VERSION_ID >= 70000
14321432
zval *value;
14331433

1434-
ZEND_HASH_FOREACH_VAL(&MONGODB_G(subscribers), value) {
1434+
ZEND_HASH_FOREACH_VAL(MONGODB_G(subscribers), value) {
14351435
/* We can't use the zend_call_method_with_1_params macro here, as it
14361436
* does a sizeof() on the name argument, which does only work with
14371437
* constant names, but not with parameterized ones as it does
@@ -1442,11 +1442,11 @@ static void php_phongo_dispatch_handlers(const char *name, zval *z_event)
14421442
HashPosition pos;
14431443
TSRMLS_FETCH();
14441444

1445-
zend_hash_internal_pointer_reset_ex(&MONGODB_G(subscribers), &pos);
1446-
for (;; zend_hash_move_forward_ex(&MONGODB_G(subscribers), &pos)) {
1445+
zend_hash_internal_pointer_reset_ex(MONGODB_G(subscribers), &pos);
1446+
for (;; zend_hash_move_forward_ex(MONGODB_G(subscribers), &pos)) {
14471447
zval **value;
14481448

1449-
if (zend_hash_get_current_data_ex(&MONGODB_G(subscribers), (void **) &value, &pos) == FAILURE) {
1449+
if (zend_hash_get_current_data_ex(MONGODB_G(subscribers), (void **) &value, &pos) == FAILURE) {
14501450
break;
14511451
}
14521452

@@ -1469,8 +1469,8 @@ static void php_phongo_command_started(const mongoc_apm_command_started_t *event
14691469
#endif
14701470
TSRMLS_FETCH();
14711471

1472-
/* Check for subscriber size */
1473-
if (zend_hash_num_elements(&MONGODB_G(subscribers)) == 0) {
1472+
/* Return early if there are no APM subscribers to notify */
1473+
if (!MONGODB_G(subscribers) || zend_hash_num_elements(MONGODB_G(subscribers)) == 0) {
14741474
return;
14751475
}
14761476

@@ -1509,8 +1509,8 @@ static void php_phongo_command_succeeded(const mongoc_apm_command_succeeded_t *e
15091509
#endif
15101510
TSRMLS_FETCH();
15111511

1512-
/* Check for subscriber size */
1513-
if (zend_hash_num_elements(&MONGODB_G(subscribers)) == 0) {
1512+
/* Return early if there are no APM subscribers to notify */
1513+
if (!MONGODB_G(subscribers) || zend_hash_num_elements(MONGODB_G(subscribers)) == 0) {
15141514
return;
15151515
}
15161516

@@ -1553,8 +1553,8 @@ static void php_phongo_command_failed(const mongoc_apm_command_failed_t *event)
15531553

15541554
default_exception_ce = zend_exception_get_default(TSRMLS_C);
15551555

1556-
/* Check for subscriber size */
1557-
if (zend_hash_num_elements(&MONGODB_G(subscribers)) == 0) {
1556+
/* Return early if there are no APM subscribers to notify */
1557+
if (!MONGODB_G(subscribers) || zend_hash_num_elements(MONGODB_G(subscribers)) == 0) {
15581558
return;
15591559
}
15601560

@@ -2091,8 +2091,12 @@ static void php_phongo_pclient_dtor(void *pp)
20912091
/* {{{ PHP_RINIT_FUNCTION */
20922092
PHP_RINIT_FUNCTION(mongodb)
20932093
{
2094-
/* Initialize HashTable for APM subscribers */
2095-
zend_hash_init(&MONGODB_G(subscribers), 0, NULL, ZVAL_PTR_DTOR, 0);
2094+
/* Initialize HashTable for APM subscribers, which is initialized to NULL in
2095+
* GINIT and destroyed and reset to NULL in RSHUTDOWN. */
2096+
if (MONGODB_G(subscribers) == NULL) {
2097+
ALLOC_HASHTABLE(MONGODB_G(subscribers));
2098+
zend_hash_init(MONGODB_G(subscribers), 0, NULL, ZVAL_PTR_DTOR, 0);
2099+
}
20962100

20972101
return SUCCESS;
20982102
}
@@ -2272,7 +2276,12 @@ PHP_MSHUTDOWN_FUNCTION(mongodb)
22722276
/* {{{ PHP_RSHUTDOWN_FUNCTION */
22732277
PHP_RSHUTDOWN_FUNCTION(mongodb)
22742278
{
2275-
zend_hash_destroy(&MONGODB_G(subscribers));
2279+
/* Destroy HashTable for APM subscribers, which was initialized in RINIT */
2280+
if (MONGODB_G(subscribers)) {
2281+
zend_hash_destroy(MONGODB_G(subscribers));
2282+
FREE_HASHTABLE(MONGODB_G(subscribers));
2283+
MONGODB_G(subscribers) = NULL;
2284+
}
22762285

22772286
return SUCCESS;
22782287
}

‎php_phongo.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ ZEND_BEGIN_MODULE_GLOBALS(mongodb)
4141
FILE *debug_fd;
4242
bson_mem_vtable_t bsonMemVTable;
4343
HashTable pclients;
44-
HashTable subscribers;
44+
HashTable *subscribers;
4545
ZEND_END_MODULE_GLOBALS(mongodb)
4646

4747
#if PHP_VERSION_ID >= 70000

‎src/MongoDB/Monitoring.c‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -52,23 +52,29 @@ PHP_FUNCTION(MongoDB_Monitoring_addSubscriber)
5252
return;
5353
}
5454

55+
/* The HashTable should never be NULL, as it's initialized during RINIT and
56+
* destroyed during RSHUTDOWN. This is simply a defensive guard. */
57+
if (!MONGODB_G(subscribers)) {
58+
return;
59+
}
60+
5561
hash = php_phongo_make_subscriber_hash(zSubscriber TSRMLS_CC);
5662
/* If we have already stored the subscriber, bail out. Otherwise, add
5763
* subscriber to list */
5864
#if PHP_VERSION_ID >= 70000
59-
if ((subscriber = zend_hash_str_find(&MONGODB_G(subscribers), hash, strlen(hash)))) {
65+
if ((subscriber = zend_hash_str_find(MONGODB_G(subscribers), hash, strlen(hash)))) {
6066
efree(hash);
6167
return;
6268
}
6369

64-
zend_hash_str_update(&MONGODB_G(subscribers), hash, strlen(hash), zSubscriber);
70+
zend_hash_str_update(MONGODB_G(subscribers), hash, strlen(hash), zSubscriber);
6571
#else
66-
if (zend_hash_find(&MONGODB_G(subscribers), hash, strlen(hash), (void**) &subscriber) == SUCCESS) {
72+
if (zend_hash_find(MONGODB_G(subscribers), hash, strlen(hash), (void**) &subscriber) == SUCCESS) {
6773
efree(hash);
6874
return;
6975
}
7076

71-
zend_hash_update(&MONGODB_G(subscribers), hash, strlen(hash), (void*) &zSubscriber, sizeof(zval*), NULL);
77+
zend_hash_update(MONGODB_G(subscribers), hash, strlen(hash), (void*) &zSubscriber, sizeof(zval*), NULL);
7278
#endif
7379
Z_ADDREF_P(zSubscriber);
7480
efree(hash);
@@ -87,12 +93,18 @@ PHP_FUNCTION(MongoDB_Monitoring_removeSubscriber)
8793
return;
8894
}
8995

96+
/* The HashTable should never be NULL, as it's initialized during RINIT and
97+
* destroyed during RSHUTDOWN. This is simply a defensive guard. */
98+
if (!MONGODB_G(subscribers)) {
99+
return;
100+
}
101+
90102
hash = php_phongo_make_subscriber_hash(zSubscriber TSRMLS_CC);
91103

92104
#if PHP_VERSION_ID >= 70000
93-
zend_hash_str_del(&MONGODB_G(subscribers), hash, strlen(hash));
105+
zend_hash_str_del(MONGODB_G(subscribers), hash, strlen(hash));
94106
#else
95-
zend_hash_del(&MONGODB_G(subscribers), hash, strlen(hash));
107+
zend_hash_del(MONGODB_G(subscribers), hash, strlen(hash));
96108
#endif
97109
efree(hash);
98110
}

‎tests/apm/bug0950-001.phpt‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
--TEST--
2+
PHPC-950: Segfault killing cursor after subscriber HashTable is destroyed (no subscribers)
3+
--SKIPIF--
4+
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
5+
<?php NEEDS('STANDALONE'); CLEANUP(STANDALONE); ?>
6+
--FILE--
7+
<?php
8+
require_once __DIR__ . "/../utils/basic.inc";
9+
10+
$manager = new MongoDB\Driver\Manager(STANDALONE);
11+
12+
$bulk = new MongoDB\Driver\BulkWrite();
13+
$bulk->insert(['_id' => 1]);
14+
$bulk->insert(['_id' => 2]);
15+
$bulk->insert(['_id' => 3]);
16+
$manager->executeBulkWrite(NS, $bulk);
17+
18+
$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query([], ['batchSize' => 2]));
19+
20+
/* Exiting during iteration on a live cursor will result in
21+
* php_phongo_command_started() being invoked for the killCursor command after
22+
* RSHUTDOWN has already destroyed the subscriber HashTable */
23+
foreach ($cursor as $data) {
24+
echo "Exiting during first iteration on cursor\n";
25+
exit(0);
26+
}
27+
28+
?>
29+
===DONE===
30+
<?php exit(0); ?>
31+
--EXPECT--
32+
Exiting during first iteration on cursor

‎tests/apm/bug0950-002.phpt‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
--TEST--
2+
PHPC-950: Segfault killing cursor after subscriber HashTable is destroyed (one subscriber)
3+
--SKIPIF--
4+
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
5+
<?php NEEDS('STANDALONE'); CLEANUP(STANDALONE); ?>
6+
--FILE--
7+
<?php
8+
require_once __DIR__ . "/../utils/basic.inc";
9+
10+
class MySubscriber implements MongoDB\Driver\Monitoring\CommandSubscriber
11+
{
12+
public function commandStarted(MongoDB\Driver\Monitoring\CommandStartedEvent $event)
13+
{
14+
printf("- started: %s\n", $event->getCommandName());
15+
}
16+
17+
public function commandSucceeded(MongoDB\Driver\Monitoring\CommandSucceededEvent $event)
18+
{
19+
printf("- succeeded: %s\n", $event->getCommandName());
20+
}
21+
22+
public function commandFailed(MongoDB\Driver\Monitoring\CommandFailedEvent $event)
23+
{
24+
printf("- failed: %s\n", $event->getCommandName());
25+
}
26+
}
27+
28+
$manager = new MongoDB\Driver\Manager(STANDALONE);
29+
30+
$bulk = new MongoDB\Driver\BulkWrite();
31+
$bulk->insert(['_id' => 1]);
32+
$bulk->insert(['_id' => 2]);
33+
$bulk->insert(['_id' => 3]);
34+
$manager->executeBulkWrite(NS, $bulk);
35+
36+
MongoDB\Monitoring\addSubscriber(new MySubscriber);
37+
38+
$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query([], ['batchSize' => 2]));
39+
40+
/* Exiting during iteration on a live cursor will result in
41+
* php_phongo_command_started() being invoked for the killCursor command after
42+
* RSHUTDOWN has already destroyed the subscriber HashTable */
43+
foreach ($cursor as $data) {
44+
echo "Exiting during first iteration on cursor\n";
45+
exit(0);
46+
}
47+
48+
?>
49+
===DONE===
50+
<?php exit(0); ?>
51+
--EXPECT--
52+
- started: find
53+
- succeeded: find
54+
Exiting during first iteration on cursor

0 commit comments

Comments
 (0)