Skip to content

PHPC:2498: Accept integer types for Document array access #1781

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
Feb 7, 2025
Merged

PHPC:2498: Accept integer types for Document array access #1781

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
eff1969
PHPC:2498: Accept integer types for Document array access
jmikola Feb 4, 2025
f8925ff
Refactor Document::get() and property handlers
jmikola Feb 4, 2025
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
117 changes: 67 additions & 50 deletions src/BSON/Document.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,36 @@ static bool php_phongo_document_get(php_phongo_document_t* intern, char* key, si
return true;
}

static bool php_phongo_document_get_by_zval(php_phongo_document_t* intern, zval* key, zval* return_value, bool null_if_missing)
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 created two by_zval variants for get and has, which are used by the ArrayAccess methods and dimension handlers. These both abstraction the zval_try_get_tmp_string logic, which was necessary to avoid possibly modifying the zval argument with a basic cast (see: Casts and Operations).

I also added a test for this using an assigned variable instead of a literal to assert that we don't modify anything inadvertently.

{
if (Z_TYPE_P(key) != IS_STRING && Z_TYPE_P(key) != IS_LONG) {
if (null_if_missing) {
ZVAL_NULL(return_value);
return true;
}

phongo_throw_exception(PHONGO_ERROR_RUNTIME, "Could not find key of type \"%s\" in BSON document", zend_zval_type_name(key));
return false;
}

zend_string* tmp_str;
zend_string* str = zval_try_get_tmp_string(key, &tmp_str);

if (!str) {
// Exception already thrown
return false;
}

if (!php_phongo_document_get(intern, ZSTR_VAL(str), ZSTR_LEN(str), return_value, null_if_missing)) {
// Exception already thrown
zend_tmp_string_release(tmp_str);
return false;
}

zend_tmp_string_release(tmp_str);
return true;
}

static PHP_METHOD(MongoDB_BSON_Document, get)
{
php_phongo_document_t* intern;
Expand All @@ -204,10 +234,8 @@ static PHP_METHOD(MongoDB_BSON_Document, get)

intern = Z_DOCUMENT_OBJ_P(getThis());

if (!php_phongo_document_get(intern, key, key_len, return_value, false)) {
// Exception already thrown
RETURN_NULL();
Copy link
Member Author

Choose a reason for hiding this comment

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

This was redundant, as null is already the default value for the return_value zval.

}
// May throw, in which case we do nothing
php_phongo_document_get(intern, key, key_len, return_value, false);
}

static PHP_METHOD(MongoDB_BSON_Document, getIterator)
Expand All @@ -229,6 +257,30 @@ static bool php_phongo_document_has(php_phongo_document_t* intern, char* key, si
return bson_iter_find_w_len(&iter, key, key_len);
}

static bool php_phongo_document_has_by_zval(php_phongo_document_t* intern, zval* key)
{
if (Z_TYPE_P(key) != IS_STRING && Z_TYPE_P(key) != IS_LONG) {
return false;
}

zend_string* tmp_str;
zend_string* str = zval_try_get_tmp_string(key, &tmp_str);
Copy link
Member Author

Choose a reason for hiding this comment

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

C99 allows declarations anywhere, but if there is a concern I can position these at the top of the function for consistency (granted, I don't think we have anything to enforce that moving forward since it's no longer a build error).


if (!str) {
// Exception already thrown
return false;
}

if (!php_phongo_document_has(intern, ZSTR_VAL(str), ZSTR_LEN(str))) {
// Exception may be thrown if BSON iterator could not be initialized
zend_tmp_string_release(tmp_str);
return false;
}

zend_tmp_string_release(tmp_str);
return true;
}

static PHP_METHOD(MongoDB_BSON_Document, has)
{
php_phongo_document_t* intern;
Expand Down Expand Up @@ -309,11 +361,7 @@ static PHP_METHOD(MongoDB_BSON_Document, offsetExists)

intern = Z_DOCUMENT_OBJ_P(getThis());

if (Z_TYPE_P(offset) != IS_STRING) {
RETURN_FALSE;
}

RETURN_BOOL(php_phongo_document_has(intern, Z_STRVAL_P(offset), Z_STRLEN_P(offset)));
RETURN_BOOL(php_phongo_document_has_by_zval(intern, offset));
}

static PHP_METHOD(MongoDB_BSON_Document, offsetGet)
Expand All @@ -327,13 +375,8 @@ static PHP_METHOD(MongoDB_BSON_Document, offsetGet)

intern = Z_DOCUMENT_OBJ_P(getThis());

if (Z_TYPE_P(offset) != IS_STRING) {
phongo_throw_exception(PHONGO_ERROR_RUNTIME, "Could not find key of type \"%s\" in BSON document", zend_zval_type_name(offset));
return;
}

// May throw, in which case we do nothing
php_phongo_document_get(intern, Z_STRVAL_P(offset), Z_STRLEN_P(offset), return_value, false);
php_phongo_document_get_by_zval(intern, offset, return_value, false);
}

static PHP_METHOD(MongoDB_BSON_Document, offsetSet)
Expand Down Expand Up @@ -550,13 +593,9 @@ static HashTable* php_phongo_document_get_properties(zend_object* object)

zval* php_phongo_document_read_property(zend_object* object, zend_string* member, int type, void** cache_slot, zval* rv)
{
php_phongo_document_t* intern;
char* key = ZSTR_VAL(member);
size_t key_len = ZSTR_LEN(member);

intern = Z_OBJ_DOCUMENT(object);
php_phongo_document_t* intern = Z_OBJ_DOCUMENT(object);

if (!php_phongo_document_get(intern, key, key_len, rv, type == BP_VAR_IS)) {
if (!php_phongo_document_get(intern, ZSTR_VAL(member), ZSTR_LEN(member), rv, type == BP_VAR_IS)) {
// Exception already thrown
return &EG(uninitialized_zval);
}
Expand All @@ -570,15 +609,11 @@ zval* php_phongo_document_write_property(zend_object* object, zend_string* membe
return value;
}

int php_phongo_document_has_property(zend_object* object, zend_string* name, int has_set_exists, void** cache_slot)
int php_phongo_document_has_property(zend_object* object, zend_string* member, int has_set_exists, void** cache_slot)
{
php_phongo_document_t* intern;
char* key = ZSTR_VAL(name);
size_t key_len = ZSTR_LEN(name);

intern = Z_OBJ_DOCUMENT(object);
php_phongo_document_t* intern = Z_OBJ_DOCUMENT(object);

return php_phongo_document_has(intern, key, key_len);
return php_phongo_document_has(intern, ZSTR_VAL(member), ZSTR_LEN(member));
}

void php_phongo_document_unset_property(zend_object* object, zend_string* member, void** cache_slot)
Expand All @@ -588,21 +623,9 @@ void php_phongo_document_unset_property(zend_object* object, zend_string* member

zval* php_phongo_document_read_dimension(zend_object* object, zval* offset, int type, zval* rv)
{
php_phongo_document_t* intern;

intern = Z_OBJ_DOCUMENT(object);

if (Z_TYPE_P(offset) != IS_STRING) {
if (type == BP_VAR_IS) {
ZVAL_NULL(rv);
return rv;
}

phongo_throw_exception(PHONGO_ERROR_RUNTIME, "Could not find key of type \"%s\" in BSON document", zend_zval_type_name(offset));
return &EG(uninitialized_zval);
}
php_phongo_document_t* intern = Z_OBJ_DOCUMENT(object);

if (!php_phongo_document_get(intern, Z_STRVAL_P(offset), Z_STRLEN_P(offset), rv, type == BP_VAR_IS)) {
if (!php_phongo_document_get_by_zval(intern, offset, rv, type == BP_VAR_IS)) {
// Exception already thrown
return &EG(uninitialized_zval);
}
Expand All @@ -617,15 +640,9 @@ void php_phongo_document_write_dimension(zend_object* object, zval* offset, zval

int php_phongo_document_has_dimension(zend_object* object, zval* member, int check_empty)
{
php_phongo_document_t* intern;

intern = Z_OBJ_DOCUMENT(object);

if (Z_TYPE_P(member) != IS_STRING) {
return false;
}
php_phongo_document_t* intern = Z_OBJ_DOCUMENT(object);

return php_phongo_document_has(intern, Z_STRVAL_P(member), Z_STRLEN_P(member));
return php_phongo_document_has_by_zval(intern, member);
}

void php_phongo_document_unset_dimension(zend_object* object, zval* offset)
Expand Down
35 changes: 35 additions & 0 deletions tests/bson/bson-document-array-access-004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
--TEST--
MongoDB\BSON\Document array access with integers (dimension object accessors)
--FILE--
<?php

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

$document = MongoDB\BSON\Document::fromPHP([
'0' => 'foo',
'1' => 'bar',
]);

// Use a variable to assert that conversion doesn't affect the original zval
$key = 1;

var_dump(isset($document[0]));
var_dump(isset($document[$key]));
var_dump(isset($document[2]));

var_dump($document[0]);
var_dump($document[$key]);

var_dump($key);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(true)
bool(true)
bool(false)
string(3) "foo"
string(3) "bar"
int(1)
===DONE===
35 changes: 35 additions & 0 deletions tests/bson/bson-document-array-access-005.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
--TEST--
MongoDB\BSON\Document array access with integers (ArrayAccess methods)
--FILE--
<?php

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

$document = MongoDB\BSON\Document::fromPHP([
'0' => 'foo',
'1' => 'bar',
]);

// Use a variable to assert that conversion doesn't affect the original zval
$key = 1;
Comment on lines +13 to +14
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, how the variable could have been updated?

Copy link
Member

Choose a reason for hiding this comment

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

Since we are operating directly on a zval, calling ZVAL_NULL(key) would update the variable, even when it wasn't passed by reference. There's no good reason to do this on purpose (despite it being used in the legacy driver), but it can happen by accident. I believe the last occurrence of us wrongly messing with the key when we were inadvertently freeing interned strings that occurred in the type map. We've gotten more careful since.

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::__construct() is another good example of this, where we need to separate the zvals to ensure any conversions don't inadvertently change the input arguments.

This is also discussed in the Casts and Operations article I linked in #1781 (comment)


var_dump($document->offsetExists(0));
var_dump($document->offsetExists($key));
var_dump($document->offsetExists(2));

var_dump($document->offsetGet(0));
var_dump($document->offsetGet($key));

var_dump($key);

?>
===DONE===
<?php exit(0); ?>
--EXPECT--
bool(true)
bool(true)
bool(false)
string(3) "foo"
string(3) "bar"
int(1)
===DONE===
6 changes: 0 additions & 6 deletions tests/bson/bson-document-array-access_error-003.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ $document = MongoDB\BSON\Document::fromPHP([
'int64' => new MongoDB\BSON\Int64(123),
]);

echo throws(function() use ($document) {
$document[0];
}, MongoDB\Driver\Exception\RuntimeException::class), "\n";

echo throws(function() use ($document) {
$document[0.1];
}, MongoDB\Driver\Exception\RuntimeException::class), "\n";
Expand All @@ -28,8 +24,6 @@ echo throws(function() use ($document) {
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\RuntimeException
Could not find key of type "int" in BSON document
OK: Got MongoDB\Driver\Exception\RuntimeException
Could not find key of type "float" in BSON document
OK: Got MongoDB\Driver\Exception\RuntimeException
Could not find key of type "bool" in BSON document
Expand Down
6 changes: 0 additions & 6 deletions tests/bson/bson-document-array-access_error-004.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@ $document = MongoDB\BSON\Document::fromPHP([
'int64' => new MongoDB\BSON\Int64(123),
]);

echo throws(function() use ($document) {
$document->offsetGet(0);
}, MongoDB\Driver\Exception\RuntimeException::class), "\n";

echo throws(function() use ($document) {
$document->offsetGet(0.1);
}, MongoDB\Driver\Exception\RuntimeException::class), "\n";
Expand All @@ -28,8 +24,6 @@ echo throws(function() use ($document) {
<?php exit(0); ?>
--EXPECT--
OK: Got MongoDB\Driver\Exception\RuntimeException
Could not find key of type "int" in BSON document
OK: Got MongoDB\Driver\Exception\RuntimeException
Could not find key of type "float" in BSON document
OK: Got MongoDB\Driver\Exception\RuntimeException
Could not find key of type "bool" in BSON document
Expand Down