PHPC-1037: Upgrade bundled libmongoc and libbson to 1.9 #672

derickr · 2017-11-14T14:24:59Z

No description provided.

jmikola · 2017-11-20T16:13:46Z

php_phongo.c

-	if (server_id > 0 && !mongoc_cursor_set_hint(cursor, server_id)) {
-		phongo_throw_exception(PHONGO_ERROR_MONGOC_FAILED TSRMLS_CC, "%s", "Could not set cursor server_id");
-		return false;
+			phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);


You can also rewrite the body of this else block as:

if (!EG(exception)) { phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC); } bson_free(opts); return false;

jmikola · 2017-11-20T17:57:05Z

php_phongo.c

+{
+	bson_t *tmp;
+
+	tmp = BCON_NEW("cursor", "{", "id", BCON_INT64(0), "ns", BCON_UTF8("db.$cmd"), "firstBatch", "[", BCON_DOCUMENT(reply), "]", "}");


Shouldn't we substitute the actual database name instead of "db" here? It doesn't seem to be relevant to passing our tests but it does seem proper.

It doesn't really matter with id=0, as the "ns" is only used for getMore I believe. I've changed it though.

jmikola · 2017-11-20T18:29:40Z

php_phongo.c


 	command = Z_COMMAND_OBJ_P(zcommand);

-	cursor = mongoc_client_command(client, db, MONGOC_QUERY_NONE, 0, 1, 0, command->bson, NULL, phongo_read_preference_from_zval(zreadPreference TSRMLS_CC));
+	opts = bson_new();
+	if (server_id > 0) {


We discussed moving server selection into Manager.c so that this function always takes a positive server_id. When that time comes, I expect this will simply append serverId to opts and we'll remove the conditional.

Just mentioning it for the record. There's no reason to make that change in this PR.

jmikola · 2017-11-20T18:34:52Z

php_phongo.c

-		phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference TSRMLS_CC);
-		return true;
+	if (!phongo_advance_cursor_and_check_for_error(cmd_cursor TSRMLS_CC)) {
+		return false;


Should we call mongoc_cursor_destroy(cmd_cursor); before returning here? There could be a leak, and I don't think we have any tests that cover an error during the first advance.

On a related note, phongo_execute_query() may also have two possible leaks in the following code blocks:

if (server_id > 0 && !mongoc_cursor_set_hint(cursor, server_id)) { phongo_throw_exception(PHONGO_ERROR_MONGOC_FAILED TSRMLS_CC, "%s", "Could not set cursor server_id"); /* THIS MAY LEAK */ return false; } /* maxAwaitTimeMS must be set before the cursor is sent */ if (query->max_await_time_ms) { mongoc_cursor_set_max_await_time_ms(cursor, query->max_await_time_ms); } if (!phongo_advance_cursor_and_check_for_error(cursor TSRMLS_CC)) { /* THIS MAY LEAK */ return false; }

The ones for phongo_execute_query should probably go into a new PR, however, we'll probably have to think hard on how to make this fail within a test case.

I've updated it here.

I'll open a ticket for phongo_execute_query: PHPC-1043.

jmikola · 2017-11-20T18:37:47Z

tests/manager/manager-ctor-auth_mechanism-002.phpt

    [null, ['authMechanism' => 'GSSAPI', 'authMechanismProperties' => ['CANONICALIZE_HOST_NAME' => 'true', 'SERVICE_NAME' => 'foo', 'SERVICE_REALM' => 'bar']]],
    // Options are case-insensitive
-    ['mongodb://127.0.0.1/?authMechanism=GSSAPI&authMechanismProperties=canonicalize_host_name:TRUE,service_name:foo,service_realm:bar', []],
+    ['mongodb://admin@127.0.0.1/?authMechanism=GSSAPI&authMechanismProperties=canonicalize_host_name:TRUE,service_name:foo,service_realm:bar', []],


I might suggest "username" for these for consistency with other placeholders. I realize the placement should make it obvious, but "admin" is often used as the default authSource.

jmikola · 2017-11-20T18:47:32Z

tests/manager/manager-ctor_error-003.phpt

-Expected string for "gssapiServiceName" URI option, array given
-OK: Got MongoDB\Driver\Exception\InvalidArgumentException
-Expected string for "gssapiServiceName" URI option, document given
-OK: Got MongoDB\Driver\Exception\InvalidArgumentException


I understand that this was removed because mongoc_uri_option_is_utf8() no longer reports true for "gssapiServiceName" (see: mongodb/mongo-c-driver@4e88488#diff-c1dfae86645148a1b148cd418f1a52f1). However, I'm concerned that we now might not be handling the option at all.

php_phongo_apply_options_to_uri() may need a new condition that specifically checks for an array option matching MONGOC_URI_GSSAPISERVICENAME and sets it on the libmongoc URI using the same logic in the aforementioned commit.

That's unfortunately not too easy, as the logic you point to uses mongoc_uri_parse_auth_mechanism_properties which is an internal function. I can of course copy all of it from src/libmongoc/src/mongoc/mongoc-uri.c, but that's not great. Can you think of anything better?

We do have mongoc_uri_set_mechanism_properties(), which works off of a bson_t * and is likely even easier to work with.

In php_phongo_apply_options_to_uri(), I expect you'll want to add a case for MONGOC_URI_GSSAPISERVICENAME that then creates a new BSON document (e.g. { "SERVICE_NAME": "user-supplied-value" }. For consistency with mongodb/mongo-c-driver@4e88488, you'll first want to check that there are no authMechanismProperties assigned before setting them from "gssapiServiceName". If mongoc_uri_get_mechanism_properties() returns true, then we have a warning condition (e.g. "authMechanismProperties SERVICE_NAME already set, ignoring '%s'"). Perhaps that means we do nothing, as I don't think the libmongoc warning would result in a PHP exception. Since PHPC doesn't provide any insight into libmongoc's URI struct (certainly not to PHP userland), I imagine we can't test this no-op behavior (i.e. "gssapiServiceName" won't override an "authMechanismProperties" option).

If mongoc_uri_get_mechanism_properties() returns false, we should then assert that the "gssapiServiceName" option is a string type. If not, we can continue to raise the unexpected type exceptions. Let's restore the original contents of the manager-ctor_error-003.phpt test.

If we do have a string, we can proceed with creating the BSON document from the "gssapiServiceName" value and assigning it with mongoc_uri_set_mechanism_properties().

derickr

I've made most of the changes, except for the one with the question. (will squash the commits into the right ones before merging, of course)

derickr · 2017-11-20T19:19:22Z

php_phongo.c

+{
+	bson_t *tmp;
+
+	tmp = BCON_NEW("cursor", "{", "id", BCON_INT64(0), "ns", BCON_UTF8("db.$cmd"), "firstBatch", "[", BCON_DOCUMENT(reply), "]", "}");


It doesn't really matter with id=0, as the "ns" is only used for getMore I believe. I've changed it though.

derickr · 2017-11-20T19:19:30Z

php_phongo.c


 	command = Z_COMMAND_OBJ_P(zcommand);

-	cursor = mongoc_client_command(client, db, MONGOC_QUERY_NONE, 0, 1, 0, command->bson, NULL, phongo_read_preference_from_zval(zreadPreference TSRMLS_CC));
+	opts = bson_new();
+	if (server_id > 0) {


derickr · 2017-11-20T19:27:07Z

php_phongo.c

-		phongo_cursor_init_for_command(return_value, client, cmd_cursor, db, zcommand, zreadPreference TSRMLS_CC);
-		return true;
+	if (!phongo_advance_cursor_and_check_for_error(cmd_cursor TSRMLS_CC)) {
+		return false;


The ones for phongo_execute_query should probably go into a new PR, however, we'll probably have to think hard on how to make this fail within a test case.

I've updated it here.

derickr · 2017-11-20T19:32:56Z

tests/manager/manager-ctor-auth_mechanism-002.phpt

    [null, ['authMechanism' => 'GSSAPI', 'authMechanismProperties' => ['CANONICALIZE_HOST_NAME' => 'true', 'SERVICE_NAME' => 'foo', 'SERVICE_REALM' => 'bar']]],
    // Options are case-insensitive
-    ['mongodb://127.0.0.1/?authMechanism=GSSAPI&authMechanismProperties=canonicalize_host_name:TRUE,service_name:foo,service_realm:bar', []],
+    ['mongodb://admin@127.0.0.1/?authMechanism=GSSAPI&authMechanismProperties=canonicalize_host_name:TRUE,service_name:foo,service_realm:bar', []],


derickr · 2017-11-20T19:58:14Z

tests/manager/manager-ctor_error-003.phpt

-Expected string for "gssapiServiceName" URI option, array given
-OK: Got MongoDB\Driver\Exception\InvalidArgumentException
-Expected string for "gssapiServiceName" URI option, document given
-OK: Got MongoDB\Driver\Exception\InvalidArgumentException


That's unfortunately not too easy, as the logic you point to uses mongoc_uri_parse_auth_mechanism_properties which is an internal function. I can of course copy all of it from src/libmongoc/src/mongoc/mongoc-uri.c, but that's not great. Can you think of anything better?

jmikola · 2017-11-20T20:31:21Z

php_phongo.c


-	tmp = BCON_NEW("cursor", "{", "id", BCON_INT64(0), "ns", BCON_UTF8("db.$cmd"), "firstBatch", "[", BCON_DOCUMENT(reply), "]", "}");
+	snprintf(ns, max_ns_len, "%s.$cmd", db);


Random thought: if db contains null bytes, I assume snprintf() would simply stop appending at the character immediately preceding the first null byte. In this case, I suppose the only side effect is that we allocated more memory for ns than was needed. More importantly, this won't affect BCON_NEW() below.

libmongoc also doesn't validate the database name when executing a command (and it also appears to assume it's null-terminated when building the command parts), so I expect it'd be up to the server to report a possible error (really only if the command required a database to exist and it did not).

It doesn't matter in any case, as the value will never be used for a getMore that might need it, as the cursorID is 0. Allocating a few more bytes won't matter either, and is certainly going to be faster than checking for NULL bytes. We don't have access to the length in phongo_execute_command either although we could of course add that (for no, gain).

jmikola · 2017-11-21T14:21:33Z

php_phongo.c

@@ -1152,6 +1152,32 @@ static bool php_phongo_apply_options_to_uri(mongoc_uri_t *uri, bson_t *options T

 			continue;
 		}
+
+		if (!strcasecmp(key, MONGOC_URI_GSSAPISERVICENAME)) {
+			bson_t dummy, properties;


tmp would be a more kosher name than dummy. We don't need to insult our structs.

properties is used below without being initialized. I imagine most compilers will allocate a stack value to zero, but it's technically undefined. I'm also not sure libbson operates with a zero-initialized struct. Initializing properties to BSON_INITIALIZER seems proper.

I tend to use "dummy" if the variable isn't used at all. It's used as option 8 at http://www.dictionary.com/browse/dummy

Will fix them though :-)

derickr force-pushed the PHPC-1037-update-to-libbson branch 3 times, most recently from d333279 to b4eaff3 Compare November 17, 2017 17:10

derickr mentioned this pull request Nov 20, 2017

PHPC-1029 Support maxTimeMS getMore option for tailable command cursors #673

Merged

derickr force-pushed the PHPC-1037-update-to-libbson branch from fa0083d to e004658 Compare November 20, 2017 16:04

jmikola reviewed Nov 20, 2017

View reviewed changes

derickr force-pushed the PHPC-1037-update-to-libbson branch from e004658 to 1b28cf8 Compare November 20, 2017 16:20

jmikola requested changes Nov 20, 2017

View reviewed changes

derickr commented Nov 20, 2017

View reviewed changes

jmikola reviewed Nov 20, 2017

View reviewed changes

jmikola mentioned this pull request Nov 20, 2017

PHPC-1046: Use authSource URI option to specify $external #675

Merged

derickr mentioned this pull request Nov 21, 2017

PHPC-995: Drop support for MongoDB 2.4 #676

Merged

jmikola requested changes Nov 21, 2017

View reviewed changes

derickr force-pushed the PHPC-1037-update-to-libbson branch from 2fb4c1e to a7311cf Compare November 21, 2017 14:42

derickr added 5 commits November 21, 2017 14:47

PHPC-1037: Upgrade bundled libmongoc and libbson to 1.9

f609ac7

PHPC-995: Drop support for MongoDB 2.4

f7cccab

PHPC-1031: Fix config.m4 action for ASN1_STRING_get0_data check

19e3bef

Replace mongoc_client_command with mongoc_client_command_with_opts

9c6b620

Add -Wmaybe-uninitialized to developer flags

3a413de

derickr force-pushed the PHPC-1037-update-to-libbson branch from a7311cf to 3a413de Compare November 21, 2017 14:47

jmikola approved these changes Nov 21, 2017

View reviewed changes

derickr merged commit 3a413de into mongodb:master Nov 21, 2017

derickr added a commit that referenced this pull request Nov 21, 2017

Merged pull request #672

9d3b99e


		tmp = BCON_NEW("cursor", "{", "id", BCON_INT64(0), "ns", BCON_UTF8("db.$cmd"), "firstBatch", "[", BCON_DOCUMENT(reply), "]", "}");
		snprintf(ns, max_ns_len, "%s.$cmd", db);

PHPC-1037: Upgrade bundled libmongoc and libbson to 1.9 #672

PHPC-1037: Upgrade bundled libmongoc and libbson to 1.9 #672

Uh oh!

Conversation

derickr commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmikola Nov 21, 2017 •

edited

Loading