Skip to content

PHPC-920: Regression test for mongoc_cursor_destroy() crash #557

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 1 commit into from
Mar 13, 2017
Merged

PHPC-920: Regression test for mongoc_cursor_destroy() crash #557

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 24 additions & 0 deletions tests/cursor/bug0920-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
PHPC-924: Using a projection with an empty field name causes a crash when destroying cursor
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; CLEANUP(STANDALONE) ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$manager = new MongoDB\Driver\Manager(STANDALONE);

$query = new MongoDB\Driver\Query([], ['projection' => ['' => 1]]);

echo throws(function() use ($manager, $query) {
$cursor = $manager->executeQuery(NS, $query);
Copy link
Member Author

Choose a reason for hiding this comment

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

With libmongoc 1.5.4, the segfault occurs here when the cursor is checked for an error, an exception is thrown, and the cursor is destroyed. In 1.5.5, the segfault is fixed but we still throw the exception here.

I think it might make more sense to do the same BSON validation that libmongoc does during cursor initialization in our own Query constructor, so we can instead throw an InvalidArgumentException earlier. This duplicates the validation logic, but the filter and options documents should be small enough where this isn't a concern. If this sound reasonable, I'll open a new ticket to address this for 1.3.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends a little bit how much code it involves. I'm generally sceptical about doing these in two places. In general, I do think it's better to have an InvalidArgumentException here.

Copy link
Member Author

@jmikola jmikola Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify, we are currently limited to a RuntimeException when executeQuery() is called, because _mongoc_cursor_new_with_opts() reports an error with the MONGOC_ERROR_CURSOR domain and MONGOC_ERROR_CURSOR_INVALID_CURSOR code. I wasn't sure if you were suggesting an InvalidArgumentException at this very point (i.e. executeQuery()), but that's not possible.

Throwing an InvalidArgumentException at Query construct time should require less than ten lines of code. I'll create a ticket and submit a PR soon.

Copy link
Member Author

@jmikola jmikola Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

See: #558

var_dump($cursor->toArray());
}, 'MongoDB\Driver\Exception\RuntimeException'), "\n";

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\RuntimeException
Cannot use empty keys in 'opts'.
===DONE===