Skip to content

PHPC-1700: Fix memory leak when read preference is invalid #1179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 25, 2020
Merged

PHPC-1700: Fix memory leak when read preference is invalid #1179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a95ca62
PHPC-1700: Fix memory leak in prep_tagsets
alcaeus Nov 18, 2020
fa6d39b
PHPC-1701: Add regression test for memory leak in prep_authmechanisms
alcaeus Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/MongoDB/ReadPreference.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,14 @@ static bool php_phongo_readpreference_init_from_hash(php_phongo_readpreference_t
}

if ((tagSets = zend_hash_str_find(props, "tags", sizeof("tags") - 1))) {
ZVAL_DEREF(tagSets);
if (Z_TYPE_P(tagSets) == IS_ARRAY) {
bson_t* tags = bson_new();

/* Separate tagSets as php_phongo_read_preference_prep_tagsets may
* modify these tags. */
SEPARATE_ZVAL_NOREF(tagSets);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping with the tradition of using ZVAL_DEREF and SEPARATE_ZVAL_NOREF. If it's more sensible to use SEPARATE_ZVAL here (and in other cases where we separate), I'd like to create a separate ticket for this.

Copy link
Member

@jmikola jmikola Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this address the following comment from PHPC-1700?

Additionally, Manager may also leak if an exception occurs after preparing the readPreferenceTags key in the URI options array. That may be possible if merge_context_options or phongo_manager_init throws.

The fix here is only for php_phongo_readpreference_init_from_hash, which Manager doesn't use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manager does not leak in that case. manager-ctor-read_prefernece-error-001.phpt already checks for this by attempting to create a manager by combining primary read preference with tags. That test did not leak when I ran it with memcheck.

The leak does not originate from throwing an exception after preparing tagsets, but from the missing zval separation in __set_state. This is also evidenced by the change necessary to fix bug1698-001.phpt, which started failing after the fix. This is because the expectation there is for the tags to be an object after calling __set_state, which shouldn't happen if we separated the zval.


php_phongo_read_preference_prep_tagsets(tagSets);
php_phongo_zval_to_bson(tagSets, PHONGO_BSON_NONE, (bson_t*) tags, NULL);

Expand Down
23 changes: 23 additions & 0 deletions tests/manager/bug1701-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
--TEST--
PHPC-1701: prep_authmechanismproperties may leak if Manager ctor errors
--FILE--
<?php

require_once __DIR__ . "/../utils/basic.inc";

echo throws(function () {
// Using a stream context without SSL options causes an exception in the constructor, triggering the potential leak
new MongoDB\Driver\Manager(
null,
['username' => 'username', 'authMechanism' => 'GSSAPI', 'authMechanismProperties' => ['canonicalize_host_name' => true]],
['context' => stream_context_create([])]
);
}, "MongoDB\Driver\Exception\InvalidArgumentException"), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\InvalidArgumentException
Stream-Context resource does not contain "ssl" options array
===DONE===
2 changes: 1 addition & 1 deletion tests/readPreference/bug1698-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ array(2) {
["tags"]=>
array(1) {
[0]=>
object(stdClass)#%d (1) {
array(1) {
["dc"]=>
string(2) "ny"
}
Expand Down