Skip to content

Commit 120b270

Browse files
jmikolajmikola
committed
PHPC-851: Separate argument zvals before modification
This primarily fixes a bug introduced in 6d46d62 for PHPC-359, since we convert tag sets from arrays to objects to ensure proper BSON serialization. Not only was the array mutation visible to calling contexts, but a crash could occur if the array was immutable due to OPcache. For Manager's $driverOptions, a comment in 1060cb8 for PHPC-433 indicated that we should have separated its zval due to possible modification, but the appropriate zend_parse_parameters() flag was never used.
1 parent 043a439 commit 120b270

File tree

6 files changed

+143
-4
lines changed

6 files changed

+143
-4
lines changed

‎php_phongo.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -837,7 +837,9 @@ void php_phongo_read_preference_prep_tagsets(zval *tagSets TSRMLS_DC) /* {{{ */
837837
zval *tagSet;
838838

839839
ZEND_HASH_FOREACH_VAL(ht_data, tagSet) {
840+
ZVAL_DEREF(tagSet);
840841
if (Z_TYPE_P(tagSet) == IS_ARRAY) {
842+
SEPARATE_ZVAL_NOREF(tagSet);
841843
convert_to_object(tagSet);
842844
}
843845
} ZEND_HASH_FOREACH_END();
@@ -851,6 +853,7 @@ void php_phongo_read_preference_prep_tagsets(zval *tagSets TSRMLS_DC) /* {{{ */
851853
zend_hash_get_current_data_ex(ht_data, (void **) &tagSet, &pos) == SUCCESS;
852854
zend_hash_move_forward_ex(ht_data, &pos)) {
853855
if (Z_TYPE_PP(tagSet) == IS_ARRAY) {
856+
SEPARATE_ZVAL_IF_NOT_REF(tagSet);
854857
convert_to_object(*tagSet);
855858
}
856859
}

‎src/MongoDB/Manager.c‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,8 @@ static void php_phongo_manager_prep_tagsets(zval *options TSRMLS_DC)
129129
/* php_phongo_make_uri() and php_phongo_apply_rp_options_to_uri()
130130
* are both case-insensitive, so we need to be as well. */
131131
if (!strcasecmp(ZSTR_VAL(string_key), "readpreferencetags")) {
132+
ZVAL_DEREF(tagSets);
133+
SEPARATE_ZVAL_NOREF(tagSets);
132134
php_phongo_read_preference_prep_tagsets(tagSets TSRMLS_CC);
133135
}
134136
} ZEND_HASH_FOREACH_END();
@@ -152,6 +154,7 @@ static void php_phongo_manager_prep_tagsets(zval *options TSRMLS_DC)
152154
/* php_phongo_make_uri() and php_phongo_apply_rp_options_to_uri()
153155
* are both case-insensitive, so we need to be as well. */
154156
if (!strcasecmp(string_key, "readpreferencetags")) {
157+
SEPARATE_ZVAL_IF_NOT_REF(tagSets);
155158
php_phongo_read_preference_prep_tagsets(*tagSets TSRMLS_CC);
156159
}
157160
}
@@ -177,9 +180,10 @@ PHP_METHOD(Manager, __construct)
177180
zend_replace_error_handling(EH_THROW, phongo_exception_from_phongo_domain(PHONGO_ERROR_INVALID_ARGUMENT), &error_handling TSRMLS_CC);
178181
intern = Z_MANAGER_OBJ_P(getThis());
179182

180-
/* Separate the driverOptions zval, since we may end up modifying it in
181-
* php_phongo_manager_merge_context_options() below. */
182-
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|s!a!a!", &uri_string, &uri_string_len, &options, &driverOptions) == FAILURE) {
183+
/* Separate the options and driverOptions zvals, since we may end up
184+
* modifying them in php_phongo_manager_prep_tagsets() and
185+
* php_phongo_manager_merge_context_options() below, respectively. */
186+
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|s!a/!a/!", &uri_string, &uri_string_len, &options, &driverOptions) == FAILURE) {
183187
zend_restore_error_handling(&error_handling TSRMLS_CC);
184188
return;
185189
}

‎src/MongoDB/ReadPreference.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,9 @@ PHP_METHOD(ReadPreference, __construct)
6363
zend_replace_error_handling(EH_THROW, phongo_exception_from_phongo_domain(PHONGO_ERROR_INVALID_ARGUMENT), &error_handling TSRMLS_CC);
6464
intern = Z_READPREFERENCE_OBJ_P(getThis());
6565

66-
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "l|a!a!", &mode, &tagSets, &options) == FAILURE) {
66+
/* Separate the tagSets zval, since we may end up modifying it in
67+
* php_phongo_read_preference_prep_tagsets() below. */
68+
if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "l|a/!a!", &mode, &tagSets, &options) == FAILURE) {
6769
zend_restore_error_handling(&error_handling TSRMLS_CC);
6870
return;
6971
}

‎tests/manager/bug0851-001.phpt‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
--TEST--
2+
PHPC-851: Manager constructor should not modify options argument
3+
--FILE--
4+
<?php
5+
6+
$options = [
7+
'readPreference' => 'secondaryPreferred',
8+
'readPreferenceTags' => [
9+
['dc' => 'ny'],
10+
[],
11+
],
12+
];
13+
14+
$manager = new MongoDB\Driver\Manager(null, $options);
15+
var_dump($options);
16+
17+
/* Dump the Manager's ReadPreference to ensure that each element in the
18+
* readPreferenceTags option was converted to an object. */
19+
var_dump($manager->getReadPreference());
20+
21+
?>
22+
===DONE===
23+
<?php exit(0); ?>
24+
--EXPECTF--
25+
array(2) {
26+
["readPreference"]=>
27+
string(18) "secondaryPreferred"
28+
["readPreferenceTags"]=>
29+
array(2) {
30+
[0]=>
31+
array(1) {
32+
["dc"]=>
33+
string(2) "ny"
34+
}
35+
[1]=>
36+
array(0) {
37+
}
38+
}
39+
}
40+
object(MongoDB\Driver\ReadPreference)#%d (%d) {
41+
["mode"]=>
42+
string(18) "secondaryPreferred"
43+
["tags"]=>
44+
array(2) {
45+
[0]=>
46+
object(stdClass)#%d (%d) {
47+
["dc"]=>
48+
string(2) "ny"
49+
}
50+
[1]=>
51+
object(stdClass)#%d (%d) {
52+
}
53+
}
54+
}
55+
===DONE===

‎tests/manager/bug0851-002.phpt‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
--TEST--
2+
PHPC-851: Manager constructor should not modify driverOptions argument
3+
--FILE--
4+
<?php
5+
6+
$driverOptions = [
7+
'weak_cert_validation' => true,
8+
'context' => stream_context_create([
9+
'ssl' => [
10+
'allow_self_signed' => true,
11+
],
12+
]),
13+
];
14+
15+
$manager = new MongoDB\Driver\Manager(null, [], $driverOptions);
16+
var_dump($driverOptions);
17+
18+
?>
19+
===DONE===
20+
<?php exit(0); ?>
21+
--EXPECT--
22+
array(2) {
23+
["weak_cert_validation"]=>
24+
bool(true)
25+
["context"]=>
26+
resource(4) of type (stream-context)
27+
}
28+
===DONE===

‎tests/readPreference/bug0851-001.phpt‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
--TEST--
2+
PHPC-851: ReadPreference constructor should not modify tagSets argument
3+
--FILE--
4+
<?php
5+
6+
$tagSets = [
7+
['dc' => 'ny'],
8+
[],
9+
];
10+
11+
$rp = new MongoDB\Driver\ReadPreference(MongoDB\Driver\ReadPreference::RP_SECONDARY_PREFERRED, $tagSets);
12+
var_dump($tagSets);
13+
14+
/* Dump the Manager's ReadPreference to ensure that each element in the $tagSets
15+
* argument was converted to an object. */
16+
var_dump($rp);
17+
18+
?>
19+
===DONE===
20+
<?php exit(0); ?>
21+
--EXPECTF--
22+
array(2) {
23+
[0]=>
24+
array(1) {
25+
["dc"]=>
26+
string(2) "ny"
27+
}
28+
[1]=>
29+
array(0) {
30+
}
31+
}
32+
object(MongoDB\Driver\ReadPreference)#%d (%d) {
33+
["mode"]=>
34+
string(18) "secondaryPreferred"
35+
["tags"]=>
36+
array(2) {
37+
[0]=>
38+
object(stdClass)#%d (%d) {
39+
["dc"]=>
40+
string(2) "ny"
41+
}
42+
[1]=>
43+
object(stdClass)#%d (%d) {
44+
}
45+
}
46+
}
47+
===DONE===

0 commit comments

Comments
 (0)