Skip to content

PHPC-1839 Fix accessing referenced strings #1223

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 4 commits into from
Jun 16, 2021
Merged

PHPC-1839 Fix accessing referenced strings #1223

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da47bb9
PHPC-1839 Fix accessing referenced non-interned strings
alcaeus May 28, 2021
453a3c4
Use ZVAL_DEREF
alcaeus Jun 2, 2021
c310135
Update tests to not require a live server
alcaeus Jun 15, 2021
66c7422
Use correct case for method names
alcaeus Jun 16, 2021
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
12 changes: 12 additions & 0 deletions src/contrib/php_array_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,12 @@ PHP_ARRAY_FETCH_TYPE_MAP(zend_bool, bool)
*/
static inline
PAA_LONG php_array_zval_to_long(zval *z) {
try_again:
if (!z) { return 0; }
switch(Z_TYPE_P(z)) {
case IS_REFERENCE:
ZVAL_DEREF(z);
goto try_again;
case IS_NULL: return 0;
#ifdef ZEND_ENGINE_3
case IS_FALSE: return 0;
Expand Down Expand Up @@ -315,8 +319,12 @@ PHP_ARRAY_FETCH_TYPE_MAP(PAA_LONG, long)
*/
static inline
double php_array_zval_to_double(zval *z) {
try_again:
if (!z) { return 0.0; }
switch (Z_TYPE_P(z)) {
case IS_REFERENCE:
ZVAL_DEREF(z);
goto try_again;
case IS_NULL: return 0.0;
#ifdef ZEND_ENGINE_3
case IS_FALSE: return 0.0;
Expand Down Expand Up @@ -357,10 +365,14 @@ static inline
char *php_array_zval_to_string(zval *z, int *plen, zend_bool *pfree) {
*plen = 0;
*pfree = 0;
try_again:
if (!z) { return NULL; }
switch (Z_TYPE_P(z)) {
case IS_NULL:
return (char *)"";
case IS_REFERENCE:
ZVAL_DEREF(z);
goto try_again;
case IS_STRING:
*plen = Z_STRLEN_P(z);
return Z_STRVAL_P(z);
Expand Down
54 changes: 54 additions & 0 deletions tests/cursor/bug1839-001.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
--TEST--
PHPC-1839: Referenced, out-of-scope, non-interned string in typeMap
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php skip_if_not_live(); ?>
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, but do any of these tests actually need a live server? Could you test the same code path using MongoDB\BSON\toPHP() and a simple BSON document (e.g. 0x05 followed by four null bytes).

If you make that change, all of these test files can be moved to the tests/bson directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated and moved. Thanks for the suggestion.

<?php skip_if_not_clean(); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

function createTypemap()
Copy link
Member Author

Choose a reason for hiding this comment

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

Using a function here to reproduce the behaviour of php-di. Notably this produces a different zval than in test 002 (see below).

{
// Assemble the string so as to not have an interned string
$rootValue = chr(ord('a')) . 'rray';
$documentValue = chr(ord('a')) . 'rray';

// Use a reference to this non-interned string in the type map
$typemap = ['root' => &$rootValue, 'document' => &$documentValue];

return $typemap;
}

$typemap = createTypemap();

$manager = new MongoDB\Driver\Manager(URI);
$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query([]));

echo "Before:\n";
debug_zval_dump($typemap);

$cursor->setTypemap($typemap);

echo "After:\n";
debug_zval_dump($typemap);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
Before:
array(2) refcount(2){
["root"]=>
string(5) "array" refcount(1)
Copy link
Member Author

Choose a reason for hiding this comment

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

Despite us assigning a reference, this is apparently converted by PHP once the variable goes out of scope. I was able to confirm this on all PHP versions: https://3v4l.org/12fdR. For this reason, I decided to keep both tests to make sure we're not running into strange behaviour anymore; this took enough time to discover already.

Copy link
Member Author

Choose a reason for hiding this comment

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

I discovered that this is indeed intended: debug_zval_dump only shows the reference indicator (&) for references with a refCount > 1.

Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify if the value of "root" (and other keys) here are still references, and this is just a matter of the behavior of php/php-src@2eb980f deciding not to render them as such?

If so, perhaps note that in a comment on the first call to debug_zval_dump() in the test.

Based on my reading of Dereferencing and Unwrapping, the unwrapping in php_array_api.h should only affect the internal zval. I'd expect the reference in the $typeMap array to remain as-is.

["document"]=>
string(5) "array" refcount(1)
}
After:
array(2) refcount(2){
["root"]=>
string(5) "array" refcount(1)
["document"]=>
string(5) "array" refcount(1)
}
===DONE===
46 changes: 46 additions & 0 deletions tests/cursor/bug1839-002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
--TEST--
PHPC-1839: Referenced local non-interned string in typeMap
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php skip_if_not_live(); ?>
<?php skip_if_not_clean(); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

// Assemble the string so as to not have an interned string
$rootValue = chr(ord('a')) . 'rray';
$documentValue = chr(ord('a')) . 'rray';

$typemap = ['root' => &$rootValue, 'document' => &$documentValue];

$manager = new MongoDB\Driver\Manager(URI);
$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query([]));

echo "Before:\n";
debug_zval_dump($typemap);

$cursor->setTypemap($typemap);

echo "After:\n";
debug_zval_dump($typemap);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
Before:
array(2) refcount(2){
["root"]=>
&string(5) "array" refcount(1)
Copy link
Member

Choose a reason for hiding this comment

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

How does debug_zval_dump() display a reference with refcount(1) in light of php/php-src@2eb980f. Wouldn't the & be suppressed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I have no clue why PHP would show the correct values here. Maybe @sgolemon can provide some insight?

["document"]=>
&string(5) "array" refcount(1)
}
After:
array(2) refcount(2){
["root"]=>
&string(5) "array" refcount(1)
["document"]=>
&string(5) "array" refcount(1)
}
===DONE===
52 changes: 52 additions & 0 deletions tests/cursor/bug1839-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
--TEST--
PHPC-1839: Referenced, out-of-scope interned string in typeMap
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php skip_if_not_live(); ?>
<?php skip_if_not_clean(); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

function createTypemap()
{
$rootValue = 'array';
$documentValue = 'array';

$typemap = ['root' => &$rootValue, 'document' => &$documentValue];

return $typemap;
}

$typemap = createTypemap();

$manager = new MongoDB\Driver\Manager(URI);
$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query([]));

echo "Before:\n";
debug_zval_dump($typemap);

$cursor->setTypemap($typemap);

echo "After:\n";
debug_zval_dump($typemap);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
Before:
array(2) refcount(2){
["root"]=>
string(5) "array" refcount(1)
Copy link
Member

Choose a reason for hiding this comment

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

Is this also a case where the values here are references, but debug_zval_dump) simply isn't displaying it as such?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this test corresponds with bug1839-001, except for the use of an interned string vs. non-interned.

["document"]=>
string(5) "array" refcount(1)
}
After:
array(2) refcount(2){
["root"]=>
string(5) "array" refcount(1)
["document"]=>
string(5) "array" refcount(1)
}
===DONE===
45 changes: 45 additions & 0 deletions tests/cursor/bug1839-004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--TEST--
PHPC-1839: Non-referenced interned string in typeMap
--SKIPIF--
<?php require __DIR__ . "/../utils/basic-skipif.inc"; ?>
<?php skip_if_not_live(); ?>
<?php skip_if_not_clean(); ?>
--FILE--
<?php
require_once __DIR__ . "/../utils/basic.inc";

$rootValue = 'array';
$documentValue = 'array';

$typemap = ['root' => &$rootValue, 'document' => &$documentValue];

$manager = new MongoDB\Driver\Manager(URI);
$cursor = $manager->executeQuery(NS, new MongoDB\Driver\Query([]));

echo "Before:\n";
debug_zval_dump($typemap);

$cursor->setTypemap($typemap);

echo "After:\n";
debug_zval_dump($typemap);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
Before:
array(2) refcount(2){
["root"]=>
&string(5) "array" refcount(1)
["document"]=>
&string(5) "array" refcount(1)
}
After:
array(2) refcount(2){
["root"]=>
&string(5) "array" refcount(1)
["document"]=>
&string(5) "array" refcount(1)
}
===DONE===