Skip to content

Commit f433820

Browse files
authored
Implement memory handling correctness fixes (#2016)
* Guard field path pop against empty path Add an early return in phongo_field_path_pop when the path has no elements to decrement. The function previously assumed callers maintained push/pop balance, which makes the helper fragile under future error-recovery edits. * Match PHP semantics for Int64 bit-shift operators ZEND_SL and ZEND_SR previously delegated directly to the C shift operators without bounding the shift amount. Match the behaviour of PHP's native integer shift handlers: throw ArithmeticError on negative shift counts, return 0 (or -1 for arithmetic right-shift of a negative value) when the count is at or above the operand width, and use an unsigned cast for the left-shift to avoid relying on signed shift semantics. * Propagate failure from phongo_zval_to_bson_value_ex Have the helper return whether the conversion populated the destination bson_value_t and forward that result through phongo_zval_to_bson_value. The IS_ARRAY/IS_OBJECT branch previously returned true unconditionally, which is misleading when an exception was thrown inside the inner encoder. * Halt BSON traversal on exception during bsonUnserialize The document and array visitors call into userland bsonUnserialize() but did not consult EG(exception) before inserting the resulting object into the parent container. When bsonUnserialize() throws, abort the traversal cleanly and propagate the exception instead of attaching a partially-constructed object to the parent. * Use a digest of the URI in the persistent client cache key The cache key produced by phongo_manager_make_client_hash previously contained the raw connection string. Replace it with a SHA-1 digest of the URI so the key remains a stable per-URI identifier without embedding the connection string verbatim into long-lived process storage. * Handle empty field path in phongo_field_path_as_string When all entries in field_path->elements are NULL, the loop appends nothing and the trailing '.' overwrite would land before the start of the allocated buffer. Branch on whether the loop wrote anything before trimming the trailing separator. * Drop tautological sparsity upper-bound check The condition sparsity > INT64_MAX is always false because sparsity is declared as int64_t. Remove the dead branch and keep the meaningful sparsity < 0 guard. * Use size_t for BSON data lengths in zval converters Change phongo_bson_data_to_zval and phongo_bson_data_to_zval_ex to accept size_t lengths instead of int, matching the unsigned width expected by libbson's reader API. Existing callers pass uint32_t from bson_iter_document, so the widening is implicit. * Use size_t for pattern and flags lengths in phongo_regex_t Bring phongo_regex_t in line with the other string-carrying structs in this file, which already use size_t for length fields. Removes a silent narrowing of the size_t parameter accepted by phongo_regex_init. * Route phongo_regex_new through phongo_regex_init phongo_regex_new (used when decoding a Regex from BSON) previously copied the pattern and flags directly without sorting the flags alphabetically, while phongo_regex_init does. This caused two Regex instances representing the same pattern and flags to compare unequal depending on which path constructed them. Defer to phongo_regex_init to keep the canonicalisation in one place. * Validate UTF-8 in scalar phongo_zval_to_bson_value path The IS_STRING branch wrote the PHP string verbatim into the bson_value_t without checking that it was valid UTF-8, while the phongo_bson_append document-encoding path already validates. Match the existing pattern so that invalid UTF-8 is rejected with an exception in both code paths. * Surface scope encoding errors from phongo_javascript_init When phongo_zval_to_bson throws while encoding the scope, release the already-allocated code buffer and the partially-written scope BSON, and report failure to the caller. Previously the function returned true even though intern was left in an inconsistent state. * Free existing buffers before re-init in BSON value classes phongo_binary_init, phongo_regex_init, and phongo_javascript_init overwrote heap-owned struct members without freeing what was already there. Re-init the slots cleanly so a second call (e.g. from a subclass that invokes parent::__unserialize twice) does not leak the prior allocation. As part of this, hoist the regex flags null-byte check above the pattern allocation so a flag rejection can no longer leave a stray pattern buffer behind. * Address PR 2016 review feedback - Restructure phongo_javascript_init to stage the new code/scope buffers in temporaries and only swap them into the object once both steps have succeeded, so a failure during scope encoding no longer leaves the object with a freed code pointer and a stale code_len. - Declare hash as an extension dependency in config.m4 and config.w32 now that phongo_client.c uses ext/hash for SHA-256, so non-default and shared builds link cleanly. - Test coverage for the new Int64 shift-bound and bson_value UTF-8 validation paths is intentionally deferred to a follow-up. - Drop hex conversion in phongo_manager_make_client_hash: use the raw 32-byte SHA-256 digest directly as the uri key in the serialized args array — PHP strings are binary-safe so the serialization output is still a stable, unique cache key - Add tests for Int64 shift-by-negative (ArithmeticError) and shift-count-at-or-above-64 (clamp to 0 / -1) - Add test for invalid UTF-8 in a scalar comment string passed to BulkWrite::__construct(), covering the scalar path in phongo_zval_to_bson_value * Update regex for matching client hashes
1 parent 332674c commit f433820

24 files changed

Lines changed: 225 additions & 49 deletions

config.m4

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -480,6 +480,7 @@ if test "$PHP_MONGODB" != "no"; then
480480
PHP_SUBST(MONGODB_SHARED_LIBADD)
481481

482482
PHP_ADD_EXTENSION_DEP(mongodb, date)
483+
PHP_ADD_EXTENSION_DEP(mongodb, hash)
483484
PHP_ADD_EXTENSION_DEP(mongodb, json)
484485
PHP_ADD_EXTENSION_DEP(mongodb, spl)
485486
PHP_ADD_EXTENSION_DEP(mongodb, standard)

config.w32

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ ARG_WITH("mongodb-client-side-encryption", "MongoDB: Enable client-side encrypti
7575

7676
if (PHP_MONGODB != "no") {
7777
ADD_EXTENSION_DEP("mongodb", "date", false);
78+
ADD_EXTENSION_DEP("mongodb", "hash", false);
7879
ADD_EXTENSION_DEP("mongodb", "standard", false);
7980
ADD_EXTENSION_DEP("mongodb", "json", false);
8081
ADD_EXTENSION_DEP("mongodb", "spl", false);

src/BSON/Binary.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ static bool phongo_binary_init(phongo_binary_t* intern, const char* data, size_t
5151
return false;
5252
}
5353

54+
if (intern->data) {
55+
efree(intern->data);
56+
}
57+
5458
intern->data = estrndup(data, data_len);
5559
intern->data_len = data_len;
5660
intern->type = (uint8_t) type;

src/BSON/Int64.c

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -443,12 +443,20 @@ static zend_result phongo_int64_do_operation_ex(zend_uchar opcode, zval* result,
443443

444444
case ZEND_SL:
445445
PHONGO_GET_INT64(value2, op2);
446-
OPERATION_RESULT_INT64(value1 << value2);
446+
if (value2 < 0) {
447+
zend_throw_exception(zend_ce_arithmetic_error, "Bit shift by negative number", 0);
448+
return FAILURE;
449+
}
450+
OPERATION_RESULT_INT64(value2 >= 64 ? 0 : (int64_t) ((uint64_t) value1 << value2));
447451
return SUCCESS;
448452

449453
case ZEND_SR:
450454
PHONGO_GET_INT64(value2, op2);
451-
OPERATION_RESULT_INT64(value1 >> value2);
455+
if (value2 < 0) {
456+
zend_throw_exception(zend_ce_arithmetic_error, "Bit shift by negative number", 0);
457+
return FAILURE;
458+
}
459+
OPERATION_RESULT_INT64(value2 >= 64 ? (value1 < 0 ? -1 : 0) : value1 >> value2);
452460
return SUCCESS;
453461

454462
case ZEND_POW:

src/BSON/Javascript.c

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ zend_class_entry* phongo_javascript_ce;
3030
* be thrown on error. */
3131
static bool phongo_javascript_init(phongo_javascript_t* intern, const char* code, size_t code_len, zval* scope)
3232
{
33+
char* new_code = NULL;
34+
bson_t* new_scope = NULL;
35+
3336
if (scope && Z_TYPE_P(scope) != IS_OBJECT && Z_TYPE_P(scope) != IS_ARRAY && Z_TYPE_P(scope) != IS_NULL) {
3437
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Expected scope to be array or object, %s given", zend_get_type_by_const(Z_TYPE_P(scope)));
3538
return false;
@@ -40,16 +43,32 @@ static bool phongo_javascript_init(phongo_javascript_t* intern, const char* code
4043
return false;
4144
}
4245

43-
intern->code = estrndup(code, code_len);
44-
intern->code_len = code_len;
46+
new_code = estrndup(code, code_len);
4547

4648
if (scope && (Z_TYPE_P(scope) == IS_OBJECT || Z_TYPE_P(scope) == IS_ARRAY)) {
47-
intern->scope = bson_new();
48-
phongo_zval_to_bson(scope, PHONGO_BSON_NONE, intern->scope, NULL);
49-
} else {
50-
intern->scope = NULL;
49+
new_scope = bson_new();
50+
phongo_zval_to_bson(scope, PHONGO_BSON_NONE, new_scope, NULL);
51+
52+
if (EG(exception)) {
53+
efree(new_code);
54+
bson_destroy(new_scope);
55+
return false;
56+
}
5157
}
5258

59+
/* Commit the new state only after all fallible steps have succeeded so a
60+
* failure leaves the object's previous contents untouched. */
61+
if (intern->code) {
62+
efree(intern->code);
63+
}
64+
if (intern->scope) {
65+
bson_destroy(intern->scope);
66+
}
67+
68+
intern->code = new_code;
69+
intern->code_len = code_len;
70+
intern->scope = new_scope;
71+
5372
return true;
5473
}
5574

src/BSON/Regex.c

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,23 @@ static bool phongo_regex_init(phongo_regex_t* intern, const char* pattern, size_
4242
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Pattern cannot contain null bytes");
4343
return false;
4444
}
45+
46+
if (flags && strlen(flags) != (size_t) flags_len) {
47+
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Flags cannot contain null bytes");
48+
return false;
49+
}
50+
51+
if (intern->pattern) {
52+
efree(intern->pattern);
53+
}
54+
if (intern->flags) {
55+
efree(intern->flags);
56+
}
57+
4558
intern->pattern = estrndup(pattern, pattern_len);
4659
intern->pattern_len = pattern_len;
4760

4861
if (flags) {
49-
if (strlen(flags) != (size_t) flags_len) {
50-
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Flags cannot contain null bytes");
51-
return false;
52-
}
5362
intern->flags = estrndup(flags, flags_len);
5463
intern->flags_len = flags_len;
5564
/* Ensure flags are alphabetized upon initialization */
@@ -296,10 +305,6 @@ void phongo_regex_init_ce(INIT_FUNC_ARGS)
296305
bool phongo_regex_new(zval* object, const char* pattern, const char* flags)
297306
{
298307
PHONGO_INTERN_INIT_EX(regex, object);
299-
intern->pattern_len = strlen(pattern);
300-
intern->pattern = estrndup(pattern, intern->pattern_len);
301-
intern->flags_len = strlen(flags);
302-
intern->flags = estrndup(flags, intern->flags_len);
303308

304-
return true;
309+
return phongo_regex_init(intern, pattern, strlen(pattern), flags, strlen(flags));
305310
}

src/MongoDB/ClientEncryption.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -856,7 +856,7 @@ static mongoc_client_encryption_encrypt_range_opts_t* phongo_clientencryption_en
856856
if (php_array_existsc(options, "sparsity")) {
857857
int64_t sparsity = php_array_fetchc_long(options, "sparsity");
858858

859-
if (sparsity < 0 || sparsity > INT64_MAX) {
859+
if (sparsity < 0) {
860860
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Expected \"sparsity\" range option to be a positive 64-bit integer, %" PRId64 " given", sparsity);
861861
goto cleanup;
862862
}

src/phongo_bson.c

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,11 @@ char* phongo_field_path_as_string(phongo_field_path* field_path)
115115
ptr++;
116116
}
117117

118-
ptr[-1] = '\0';
118+
if (ptr == path) {
119+
path[0] = '\0';
120+
} else {
121+
ptr[-1] = '\0';
122+
}
119123

120124
return path;
121125
}
@@ -205,6 +209,10 @@ bool phongo_field_path_push(phongo_field_path* field_path, const char* element,
205209

206210
bool phongo_field_path_pop(phongo_field_path* field_path)
207211
{
212+
if (field_path->size == 0) {
213+
return false;
214+
}
215+
208216
phongo_field_path_ensure_allocation(field_path, field_path->size);
209217

210218
field_path->elements[field_path->size] = NULL;
@@ -783,6 +791,15 @@ static bool phongo_bson_visit_document(const bson_iter_t* iter ARG_UNUSED, const
783791
object_init_ex(&obj, obj_ce);
784792

785793
zend_call_method_with_1_params(Z_OBJ_P(&obj), NULL, NULL, BSON_UNSERIALIZE_FUNC_NAME, NULL, &state.zchild);
794+
795+
if (EG(exception)) {
796+
zval_ptr_dtor(&obj);
797+
zval_ptr_dtor(&state.zchild);
798+
phongo_bson_state_dtor(&state);
799+
phongo_field_path_pop(parent_state->field_path);
800+
return true;
801+
}
802+
786803
zval_ptr_dtor(&state.zchild);
787804
ZVAL_COPY_VALUE(&state.zchild, &obj);
788805

@@ -859,6 +876,15 @@ static bool phongo_bson_visit_array(const bson_iter_t* iter ARG_UNUSED, const ch
859876

860877
object_init_ex(&obj, state.field_type.ce);
861878
zend_call_method_with_1_params(Z_OBJ_P(&obj), NULL, NULL, BSON_UNSERIALIZE_FUNC_NAME, NULL, &state.zchild);
879+
880+
if (EG(exception)) {
881+
zval_ptr_dtor(&obj);
882+
zval_ptr_dtor(&state.zchild);
883+
phongo_bson_state_dtor(&state);
884+
phongo_field_path_pop(parent_state->field_path);
885+
return true;
886+
}
887+
862888
zval_ptr_dtor(&state.zchild);
863889
ZVAL_COPY_VALUE(&state.zchild, &obj);
864890
break;
@@ -901,7 +927,7 @@ bool phongo_bson_to_zval(const bson_t* b, zval* zv)
901927
}
902928

903929
/* Converts BSON data to a PHP value using the default typemap. */
904-
bool phongo_bson_data_to_zval(const unsigned char* data, int data_len, zval* zv)
930+
bool phongo_bson_data_to_zval(const unsigned char* data, size_t data_len, zval* zv)
905931
{
906932
bool retval;
907933
phongo_bson_state state;
@@ -1183,7 +1209,7 @@ bool phongo_bson_to_zval_ex(const bson_t* b, phongo_bson_state* state)
11831209
* as-is on PHP 7; however, it should have the type undefined if the state
11841210
* was initialized to zero.
11851211
*/
1186-
bool phongo_bson_data_to_zval_ex(const unsigned char* data, int data_len, phongo_bson_state* state)
1212+
bool phongo_bson_data_to_zval_ex(const unsigned char* data, size_t data_len, phongo_bson_state* state)
11871213
{
11881214
bson_reader_t* reader = NULL;
11891215
const bson_t* b;

src/phongo_bson.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,8 @@ bool phongo_field_path_pop(phongo_field_path* field_path);
109109
bool phongo_bson_to_json(zval* return_value, const bson_t* bson, phongo_json_mode_t mode);
110110
bool phongo_bson_to_zval(const bson_t* b, zval* zv);
111111
bool phongo_bson_to_zval_ex(const bson_t* b, phongo_bson_state* state);
112-
bool phongo_bson_data_to_zval(const unsigned char* data, int data_len, zval* zv);
113-
bool phongo_bson_data_to_zval_ex(const unsigned char* data, int data_len, phongo_bson_state* state);
112+
bool phongo_bson_data_to_zval(const unsigned char* data, size_t data_len, zval* zv);
113+
bool phongo_bson_data_to_zval_ex(const unsigned char* data, size_t data_len, phongo_bson_state* state);
114114

115115
bool phongo_bson_value_to_zval(const bson_value_t* value, zval* zv);
116116
bool phongo_bson_value_to_zval_legacy(const bson_value_t* value, zval* zv);

src/phongo_bson_encode.c

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -571,11 +571,12 @@ void phongo_zval_to_bson(zval* data, phongo_bson_flags_t flags, bson_t* bson, bs
571571
phongo_field_path_free(field_path);
572572
}
573573

574-
static void phongo_zval_to_bson_value_ex(zval* data, phongo_bson_flags_t flags, bson_value_t* value)
574+
static bool phongo_zval_to_bson_value_ex(zval* data, phongo_bson_flags_t flags, bson_value_t* value)
575575
{
576576
bson_iter_t iter;
577577
bson_t bson = BSON_INITIALIZER;
578578
zval data_object;
579+
bool success = false;
579580

580581
array_init_size(&data_object, 1);
581582
add_assoc_zval(&data_object, "data", data);
@@ -584,12 +585,15 @@ static void phongo_zval_to_bson_value_ex(zval* data, phongo_bson_flags_t flags,
584585

585586
phongo_zval_to_bson(&data_object, flags, &bson, NULL);
586587

587-
if (bson_iter_init_find(&iter, &bson, "data")) {
588+
if (!EG(exception) && bson_iter_init_find(&iter, &bson, "data")) {
588589
bson_value_copy(bson_iter_value(&iter), value);
590+
success = true;
589591
}
590592

591593
bson_destroy(&bson);
592594
zval_ptr_dtor(&data_object);
595+
596+
return success;
593597
}
594598

595599
/* Converts the argument to a bson_value_t. If the object is an instance of
@@ -639,6 +643,11 @@ bool phongo_zval_to_bson_value(zval* data, bson_value_t* value)
639643
return true;
640644

641645
case IS_STRING:
646+
if (!bson_utf8_validate(Z_STRVAL_P(data), Z_STRLEN_P(data), true)) {
647+
phongo_throw_exception(PHONGO_ERROR_UNEXPECTED_VALUE, "Detected invalid UTF-8 in string value");
648+
return false;
649+
}
650+
642651
value->value_type = BSON_TYPE_UTF8;
643652
value->value.v_utf8.len = Z_STRLEN_P(data);
644653

@@ -651,8 +660,7 @@ bool phongo_zval_to_bson_value(zval* data, bson_value_t* value)
651660
case IS_ARRAY:
652661
case IS_OBJECT:
653662
/* Use phongo_zval_to_bson internally to convert arrays and documents */
654-
phongo_zval_to_bson_value_ex(data, PHONGO_BSON_NONE, value);
655-
return true;
663+
return phongo_zval_to_bson_value_ex(data, PHONGO_BSON_NONE, value);
656664
}
657665

658666
phongo_throw_exception(PHONGO_ERROR_INVALID_ARGUMENT, "Unsupported type %s", zend_zval_type_name(data));

0 commit comments

Comments
 (0)