From 06f608059e8c46d2145000e8c1ca5f3ec5bae7a4 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 12 Apr 2018 12:19:49 -0400 Subject: [PATCH 1/3] Move up client argument for phongo_parse_session() Client is required, while the opts and zval arguments are optional. --- php_phongo.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/php_phongo.c b/php_phongo.c index 45e665b8d..8a68148f3 100644 --- a/php_phongo.c +++ b/php_phongo.c @@ -540,11 +540,13 @@ bool phongo_parse_read_preference(zval *options, zval **zreadPreference TSRMLS_D return true; } /* }}} */ -/* Parses the "session" option for an execute method. If mongoc_opts is not - * NULL, the option will be appended. If zsession is not NULL, it will be +/* Parses the "session" option for an execute method. The client object should + * correspond to the Manager executing the operation and will be used to ensure + * that the session is correctly associated with that client. If mongoc_opts is + * not NULL, the option will be appended. If zsession is not NULL, it will be * assigned to the option. On error, false is returned and an exception is * thrown. */ -static bool phongo_parse_session(zval *options, bson_t *mongoc_opts, zval **zsession, mongoc_client_t *client TSRMLS_DC) /* {{{ */ +static bool phongo_parse_session(zval *options, mongoc_client_t *client, bson_t *mongoc_opts, zval **zsession TSRMLS_DC) /* {{{ */ { zval *option = NULL; const mongoc_client_session_t *client_session; @@ -652,7 +654,7 @@ bool phongo_execute_bulk_write(mongoc_client_t *client, const char *namespace, p return false; } - if (!phongo_parse_session(options, NULL, &zsession, client TSRMLS_CC)) { + if (!phongo_parse_session(options, client, NULL, &zsession TSRMLS_CC)) { /* Exception should already have been thrown */ return false; } @@ -767,7 +769,7 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z return false; } - if (!phongo_parse_session(options, query->opts, NULL, client TSRMLS_CC)) { + if (!phongo_parse_session(options, client, query->opts, NULL TSRMLS_CC)) { /* Exception should already have been thrown */ mongoc_collection_destroy(collection); return false; @@ -839,7 +841,7 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty return false; } - if (!phongo_parse_session(options, &opts, NULL, client TSRMLS_CC)) { + if (!phongo_parse_session(options, client, &opts, NULL TSRMLS_CC)) { /* Exception should already have been thrown */ bson_destroy(&opts); return false; From 21307dee76b021c48d90b6e997e459792da8ce0d Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 12 Apr 2018 12:22:53 -0400 Subject: [PATCH 2/3] PHPC-1151: Maintain Session reference on Cursor This ensures that the Session and underlying mongoc_client_session_t will not be freed until after the Cursor's mongoc_cursor_t object is destroyed. --- php_phongo.c | 30 +++++++++++++++++-------- php_phongo_structs.h | 1 + src/MongoDB/Cursor.c | 4 ++++ tests/cursor/bug1151-001.phpt | 38 +++++++++++++++++++++++++++++++ tests/cursor/bug1151-002.phpt | 42 +++++++++++++++++++++++++++++++++++ tests/cursor/bug1151-003.phpt | 32 ++++++++++++++++++++++++++ tests/cursor/bug1151-004.phpt | 36 ++++++++++++++++++++++++++++++ 7 files changed, 174 insertions(+), 9 deletions(-) create mode 100644 tests/cursor/bug1151-001.phpt create mode 100644 tests/cursor/bug1151-002.phpt create mode 100644 tests/cursor/bug1151-003.phpt create mode 100644 tests/cursor/bug1151-004.phpt diff --git a/php_phongo.c b/php_phongo.c index 8a68148f3..1fcf2db02 100644 --- a/php_phongo.c +++ b/php_phongo.c @@ -238,7 +238,7 @@ static void php_phongo_log(mongoc_log_level_t log_level, const char *log_domain, /* }}} */ /* {{{ Init objects */ -static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, zval *readPreference TSRMLS_DC) /* {{{ */ +static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, zval *readPreference, zval *session TSRMLS_DC) /* {{{ */ { php_phongo_cursor_t *intern; @@ -256,15 +256,24 @@ static void phongo_cursor_init(zval *return_value, mongoc_client_t *client, mong #else Z_ADDREF_P(readPreference); intern->read_preference = readPreference; +#endif + } + + if (session) { +#if PHP_VERSION_ID >= 70000 + ZVAL_ZVAL(&intern->session, session, 1, 0); +#else + Z_ADDREF_P(session); + intern->session = session; #endif } } /* }}} */ -static void phongo_cursor_init_for_command(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *db, zval *command, zval *readPreference TSRMLS_DC) /* {{{ */ +static void phongo_cursor_init_for_command(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *db, zval *command, zval *readPreference, zval *session TSRMLS_DC) /* {{{ */ { php_phongo_cursor_t *intern; - phongo_cursor_init(return_value, client, cursor, readPreference TSRMLS_CC); + phongo_cursor_init(return_value, client, cursor, readPreference, session TSRMLS_CC); intern = Z_CURSOR_OBJ_P(return_value); intern->database = estrdup(db); @@ -277,11 +286,11 @@ static void phongo_cursor_init_for_command(zval *return_value, mongoc_client_t * #endif } /* }}} */ -static void phongo_cursor_init_for_query(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *namespace, zval *query, zval *readPreference TSRMLS_DC) /* {{{ */ +static void phongo_cursor_init_for_query(zval *return_value, mongoc_client_t *client, mongoc_cursor_t *cursor, const char *namespace, zval *query, zval *readPreference, zval *session TSRMLS_DC) /* {{{ */ { php_phongo_cursor_t *intern; - phongo_cursor_init(return_value, client, cursor, readPreference TSRMLS_CC); + phongo_cursor_init(return_value, client, cursor, readPreference, session TSRMLS_CC); intern = Z_CURSOR_OBJ_P(return_value); /* namespace has already been validated by phongo_execute_query() */ @@ -748,6 +757,7 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z char *collname; mongoc_collection_t *collection; zval *zreadPreference = NULL; + zval *zsession = NULL; if (!phongo_split_namespace(namespace, &dbname, &collname)) { phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT TSRMLS_CC, "%s: %s", "Invalid namespace provided", namespace); @@ -769,7 +779,7 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z return false; } - if (!phongo_parse_session(options, client, query->opts, NULL TSRMLS_CC)) { + if (!phongo_parse_session(options, client, query->opts, &zsession TSRMLS_CC)) { /* Exception should already have been thrown */ mongoc_collection_destroy(collection); return false; @@ -799,7 +809,8 @@ int phongo_execute_query(mongoc_client_t *client, const char *namespace, zval *z return true; } - phongo_cursor_init_for_query(return_value, client, cursor, namespace, zquery, zreadPreference TSRMLS_CC); + phongo_cursor_init_for_query(return_value, client, cursor, namespace, zquery, zreadPreference, zsession TSRMLS_CC); + return true; } /* }}} */ @@ -825,6 +836,7 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty bson_t opts = BSON_INITIALIZER; mongoc_cursor_t *cmd_cursor; zval *zreadPreference = NULL; + zval *zsession = NULL; int result; command = Z_COMMAND_OBJ_P(zcommand); @@ -841,7 +853,7 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty return false; } - if (!phongo_parse_session(options, client, &opts, NULL TSRMLS_CC)) { + if (!phongo_parse_session(options, client, &opts, &zsession TSRMLS_CC)) { /* Exception should already have been thrown */ bson_destroy(&opts); return false; @@ -922,7 +934,7 @@ int phongo_execute_command(mongoc_client_t *client, php_phongo_command_type_t ty bson_destroy(&reply); } - phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference TSRMLS_CC); + phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference, zsession TSRMLS_CC); return true; } /* }}} */ /* }}} */ diff --git a/php_phongo_structs.h b/php_phongo_structs.h index d96a3953e..641892bff 100644 --- a/php_phongo_structs.h +++ b/php_phongo_structs.h @@ -65,6 +65,7 @@ typedef struct { PHONGO_STRUCT_ZVAL query; PHONGO_STRUCT_ZVAL command; PHONGO_STRUCT_ZVAL read_preference; + PHONGO_STRUCT_ZVAL session; PHONGO_ZEND_OBJECT_POST } php_phongo_cursor_t; diff --git a/src/MongoDB/Cursor.c b/src/MongoDB/Cursor.c index 68ecae4b9..3284d9d77 100644 --- a/src/MongoDB/Cursor.c +++ b/src/MongoDB/Cursor.c @@ -392,6 +392,10 @@ static void php_phongo_cursor_free_object(phongo_free_object_arg *object TSRMLS_ zval_ptr_dtor(&intern->read_preference); } + if (!Z_ISUNDEF(intern->session)) { + zval_ptr_dtor(&intern->session); + } + php_phongo_cursor_free_current(intern); #if PHP_VERSION_ID < 70000 diff --git a/tests/cursor/bug1151-001.phpt b/tests/cursor/bug1151-001.phpt new file mode 100644 index 000000000..cc0eea624 --- /dev/null +++ b/tests/cursor/bug1151-001.phpt @@ -0,0 +1,38 @@ +--TEST-- +PHPC-1151: Segfault if session unset before first getMore (find) +--SKIPIF-- + + + + + +--FILE-- +insert(['_id' => 1]); +$bulk->insert(['_id' => 2]); +$bulk->insert(['_id' => 3]); +$manager->executeBulkWrite(NS, $bulk); + +$query = new MongoDB\Driver\Query([], ['batchSize' => 2]); +$session = $manager->startSession(); + +$cursor = $manager->executeQuery(NS, $query, ['session' => $session]); + +foreach ($cursor as $document) { + unset($session); + echo $document->_id, "\n"; +} + +?> +===DONE=== + +--EXPECT-- +1 +2 +3 +===DONE=== diff --git a/tests/cursor/bug1151-002.phpt b/tests/cursor/bug1151-002.phpt new file mode 100644 index 000000000..1028c1820 --- /dev/null +++ b/tests/cursor/bug1151-002.phpt @@ -0,0 +1,42 @@ +--TEST-- +PHPC-1151: Segfault if session unset before first getMore (aggregate) +--SKIPIF-- + + + + + +--FILE-- +insert(['_id' => 1]); +$bulk->insert(['_id' => 2]); +$bulk->insert(['_id' => 3]); +$manager->executeBulkWrite(NS, $bulk); + +$command = new MongoDB\Driver\Command([ + 'aggregate' => COLLECTION_NAME, + 'pipeline' => [], + 'cursor' => ['batchSize' => 2], +]); +$session = $manager->startSession(); + +$cursor = $manager->executeReadCommand(DATABASE_NAME, $command, ['session' => $session]); + +foreach ($cursor as $document) { + unset($session); + echo $document->_id, "\n"; +} + +?> +===DONE=== + +--EXPECT-- +1 +2 +3 +===DONE=== diff --git a/tests/cursor/bug1151-003.phpt b/tests/cursor/bug1151-003.phpt new file mode 100644 index 000000000..623da93a4 --- /dev/null +++ b/tests/cursor/bug1151-003.phpt @@ -0,0 +1,32 @@ +--TEST-- +PHPC-1151: Segfault if session unset before cursor is killed (find) +--SKIPIF-- + + + + + +--FILE-- +insert(['_id' => 1]); +$bulk->insert(['_id' => 2]); +$bulk->insert(['_id' => 3]); +$manager->executeBulkWrite(NS, $bulk); + +$query = new MongoDB\Driver\Query([], ['batchSize' => 2]); +$session = $manager->startSession(); + +$cursor = $manager->executeQuery(NS, $query, ['session' => $session]); +unset($session); +unset($cursor); + +?> +===DONE=== + +--EXPECT-- +===DONE=== diff --git a/tests/cursor/bug1151-004.phpt b/tests/cursor/bug1151-004.phpt new file mode 100644 index 000000000..27d7462bf --- /dev/null +++ b/tests/cursor/bug1151-004.phpt @@ -0,0 +1,36 @@ +--TEST-- +PHPC-1151: Segfault if session unset before cursor is killed (aggregate) +--SKIPIF-- + + + + + +--FILE-- +insert(['_id' => 1]); +$bulk->insert(['_id' => 2]); +$bulk->insert(['_id' => 3]); +$manager->executeBulkWrite(NS, $bulk); + +$command = new MongoDB\Driver\Command([ + 'aggregate' => COLLECTION_NAME, + 'pipeline' => [], + 'cursor' => ['batchSize' => 2], +]); +$session = $manager->startSession(); + +$cursor = $manager->executeReadCommand(DATABASE_NAME, $command, ['session' => $session]); +unset($session); +unset($cursor); + +?> +===DONE=== + +--EXPECT-- +===DONE=== From cc9030465881533036cfb9b60fc077de116f3964 Mon Sep 17 00:00:00 2001 From: Jeremy Mikola Date: Thu, 12 Apr 2018 13:17:08 -0400 Subject: [PATCH 3/3] PHPC-1151: Add Session to Cursor debug output --- src/MongoDB/Cursor.c | 12 ++++++++++++ tests/manager/manager-executeCommand-001.phpt | 2 ++ tests/manager/manager-executeQuery-001.phpt | 2 ++ tests/manager/manager-executeQuery-002.phpt | 2 ++ tests/readPreference/bug0146-001.phpt | 10 ++++++++++ tests/readPreference/bug0146-002.phpt | 10 ++++++++++ tests/server/server-executeCommand-001.phpt | 2 ++ 7 files changed, 40 insertions(+) diff --git a/src/MongoDB/Cursor.c b/src/MongoDB/Cursor.c index 3284d9d77..8be94a69b 100644 --- a/src/MongoDB/Cursor.c +++ b/src/MongoDB/Cursor.c @@ -490,6 +490,18 @@ static HashTable *php_phongo_cursor_get_debug_info(zval *object, int *is_temp TS ADD_ASSOC_NULL_EX(&retval, "readPreference"); } + if (!Z_ISUNDEF(intern->session)) { +#if PHP_VERSION_ID >= 70000 + ADD_ASSOC_ZVAL_EX(&retval, "session", &intern->session); + Z_ADDREF(intern->session); +#else + ADD_ASSOC_ZVAL_EX(&retval, "session", intern->session); + Z_ADDREF_P(intern->session); +#endif + } else { + ADD_ASSOC_NULL_EX(&retval, "session"); + } + ADD_ASSOC_BOOL_EX(&retval, "isDead", !mongoc_cursor_is_alive(intern->cursor)); ADD_ASSOC_LONG_EX(&retval, "currentIndex", intern->current); diff --git a/tests/manager/manager-executeCommand-001.phpt b/tests/manager/manager-executeCommand-001.phpt index f0f40e442..5439ed573 100644 --- a/tests/manager/manager-executeCommand-001.phpt +++ b/tests/manager/manager-executeCommand-001.phpt @@ -54,6 +54,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { } ["readPreference"]=> NULL + ["session"]=> + NULL ["isDead"]=> bool(false) ["currentIndex"]=> diff --git a/tests/manager/manager-executeQuery-001.phpt b/tests/manager/manager-executeQuery-001.phpt index a494402d2..f5052971c 100644 --- a/tests/manager/manager-executeQuery-001.phpt +++ b/tests/manager/manager-executeQuery-001.phpt @@ -64,6 +64,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { NULL ["readPreference"]=> NULL + ["session"]=> + NULL ["isDead"]=> bool(false) ["currentIndex"]=> diff --git a/tests/manager/manager-executeQuery-002.phpt b/tests/manager/manager-executeQuery-002.phpt index 8bf81c9b0..771599789 100644 --- a/tests/manager/manager-executeQuery-002.phpt +++ b/tests/manager/manager-executeQuery-002.phpt @@ -64,6 +64,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { NULL ["readPreference"]=> NULL + ["session"]=> + NULL ["isDead"]=> bool(false) ["currentIndex"]=> diff --git a/tests/readPreference/bug0146-001.phpt b/tests/readPreference/bug0146-001.phpt index 9e7b81576..2d4ebe8fa 100644 --- a/tests/readPreference/bug0146-001.phpt +++ b/tests/readPreference/bug0146-001.phpt @@ -58,6 +58,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(7) "primary" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -96,6 +98,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(16) "primaryPreferred" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -134,6 +138,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(9) "secondary" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -172,6 +178,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(18) "secondaryPreferred" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -210,6 +218,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(7) "nearest" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> diff --git a/tests/readPreference/bug0146-002.phpt b/tests/readPreference/bug0146-002.phpt index a1a35a93d..fc384066c 100644 --- a/tests/readPreference/bug0146-002.phpt +++ b/tests/readPreference/bug0146-002.phpt @@ -58,6 +58,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(7) "primary" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -96,6 +98,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(16) "primaryPreferred" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -134,6 +138,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(9) "secondary" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -172,6 +178,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(18) "secondaryPreferred" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> @@ -210,6 +218,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { ["mode"]=> string(7) "nearest" } + ["session"]=> + NULL ["isDead"]=> bool(true) ["currentIndex"]=> diff --git a/tests/server/server-executeCommand-001.phpt b/tests/server/server-executeCommand-001.phpt index f114f65d6..d5ee14e8c 100644 --- a/tests/server/server-executeCommand-001.phpt +++ b/tests/server/server-executeCommand-001.phpt @@ -43,6 +43,8 @@ object(MongoDB\Driver\Cursor)#%d (%d) { } ["readPreference"]=> NULL + ["session"]=> + NULL ["isDead"]=> bool(false) ["currentIndex"]=>