PHPC-531, PHPC-616: Fix segfault and improve error reporting for corrupt BSON

src/bson.c

@@ -179,21 +179,9 @@ bool php_phongo_bson_visit_after(const bson_iter_t *iter ARG_UNUSED, const char
 }
 /* }}} */
 #endif
-void php_phongo_bson_visit_corrupt(const bson_iter_t *iter ARG_UNUSED, void *data) /* {{{ */
+void php_phongo_bson_visit_corrupt(const bson_iter_t *iter ARG_UNUSED, void *data ARG_UNUSED) /* {{{ */
 {
-#if PHP_VERSION_ID >= 70000
-	zval *retval = &((php_phongo_bson_state *)data)->zchild;
-#else
-	zval *retval = ((php_phongo_bson_state *)data)->zchild;
-#endif
-
 	mongoc_log(MONGOC_LOG_LEVEL_TRACE, MONGOC_LOG_DOMAIN, "Corrupt BSON data detected!");
-
-#if PHP_VERSION_ID >= 70000
-	zval_ptr_dtor(retval);
-#else
-	zval_ptr_dtor(&retval);
-#endif
 }
 /* }}} */
 bool php_phongo_bson_visit_double(const bson_iter_t *iter ARG_UNUSED, const char *key, double v_double, void *data) /* {{{ */
@@ -612,7 +600,7 @@ bool php_phongo_bson_visit_document(const bson_iter_t *iter ARG_UNUSED, const ch
 		array_init(state.zchild);
 #endif
 
-		if (!bson_iter_visit_all(&child, &php_bson_visitors, &state)) {
+		if (!bson_iter_visit_all(&child, &php_bson_visitors, &state) && !child.err_off) {
 			/* If php_phongo_bson_visit_binary() finds an ODM class, it should
 			 * supersede a default type map and named document class. */
 			if (state.odm && state.map.document_type == PHONGO_TYPEMAP_NONE) {
@@ -662,6 +650,12 @@ bool php_phongo_bson_visit_document(const bson_iter_t *iter ARG_UNUSED, const ch
 					Z_SET_REFCOUNT_P(state.zchild, 1);
 #endif
 			}
+		} else {
+			/* Iteration stopped prematurely due to corruption or a failed
+			 * visitor. Free state.zchild, which we just initialized, and return
+			 * true to stop iteration for our parent context. */
+			zval_ptr_dtor(&state.zchild);
+			return true;
 		}
 	}
 
@@ -691,7 +685,7 @@ bool php_phongo_bson_visit_array(const bson_iter_t *iter ARG_UNUSED, const char
 		array_init(state.zchild);
 #endif
 
-		if (!bson_iter_visit_all(&child, &php_bson_visitors, &state)) {
+		if (!bson_iter_visit_all(&child, &php_bson_visitors, &state) && !child.err_off) {
 
 			switch(state.map.array_type) {
 				case PHONGO_TYPEMAP_CLASS: {
@@ -737,6 +731,12 @@ bool php_phongo_bson_visit_array(const bson_iter_t *iter ARG_UNUSED, const char
 #endif
 					break;
 			}
+		} else {
+			/* Iteration stopped prematurely due to corruption or a failed
+			 * visitor. Free state.zchild, which we just initialized, and return
+			 * true to stop iteration for our parent context. */
+			zval_ptr_dtor(&state.zchild);
+			return true;
 		}
 
 	}
@@ -1385,7 +1385,15 @@ PHONGO_API int phongo_bson_to_zval_ex(const unsigned char *data, int data_len, p
 #else
 	array_init(state->zchild);
 #endif
-	bson_iter_visit_all(&iter, &php_bson_visitors, state);
+
+	if (bson_iter_visit_all(&iter, &php_bson_visitors, state) || iter.err_off) {
+		/* Iteration stopped prematurely due to corruption or a failed visitor.
+		 * While we free the reader, state->zchild should be left as-is, since
+		 * the calling code may want to zval_ptr_dtor() it. */
+		phongo_throw_exception(PHONGO_ERROR_UNEXPECTED_VALUE TSRMLS_CC, "Could not convert BSON document to a PHP variable");
+		bson_reader_destroy(reader);
+		return 0;
+	}
 
 	/* If php_phongo_bson_visit_binary() finds an ODM class, it should supersede
 	 * a default type map and named root class. */

tests/bson/bson-toPHP_error-003.phpt

@@ -0,0 +1,28 @@
+--TEST--
+BSON\toPHP(): BSON decoding exceptions for malformed documents
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"?>
+--FILE--
+<?php
+require_once __DIR__ . "/../utils/basic.inc";
+
+$tests = array(
+    pack('Vx', 4), // Empty document with invalid length (too small)
+    pack('Vx', 6), // Empty document with invalid length (too large)
+);
+
+foreach ($tests as $bson) {
+    echo throws(function() use ($bson) {
+        toPHP($bson);
+    }, 'MongoDB\Driver\Exception\UnexpectedValueException'), "\n";
+}
+
+?>
+===DONE===
+<?php exit(0); ?>
+--EXPECTF--
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not read document from BSON reader
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not read document from BSON reader
+===DONE===

tests/bson/bson-toPHP_error-004.phpt

@@ -0,0 +1,47 @@
+--TEST--
+BSON\toPHP(): BSON decoding exceptions for bson_iter_visit_all() failure
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"?>
+--FILE--
+<?php
+require_once __DIR__ . "/../utils/basic.inc";
+
+$tests = array(
+    // Invalid UTF-8 character in root document's field name
+    str_replace('INVALID!', "INVALID\xFE", fromPHP(['INVALID!' => 'bar'])),
+    // Invalid UTF-8 character in embedded document's field name
+    str_replace('INVALID!', "INVALID\xFE", fromPHP(['foo' => ['INVALID!' => 'bar']])),
+    // Invalid UTF-8 character in string within array field
+    str_replace('INVALID!', "INVALID\xFE", fromPHP(['foo' => ['INVALID!']])),
+    /* Note: we don't use a three-character string in the underflow case, as
+     * the 4-byte string length and payload (i.e. three characters + null byte)
+     * coincidentally satisfy the expected size for an 8-byte double. We also
+     * don't use a four-character string, since its null byte would be
+     * interpreted as the document terminator. The actual document terminator
+     * would then remain in the buffer and trigger a "did not exhaust" error.
+     */
+    pack('VCa*xVa*xx', 17, 1, 'foo', 3, 'ab'), // Invalid field type (underflow)
+    pack('VCa*xVa*xx', 20, 1, 'foo', 6, 'abcde'), // Invalid field type (overflow)
+);
+
+foreach ($tests as $bson) {
+    echo throws(function() use ($bson) {
+        toPHP($bson);
+    }, 'MongoDB\Driver\Exception\UnexpectedValueException'), "\n";
+}
+
+?>
+===DONE===
+<?php exit(0); ?>
+--EXPECTF--
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not convert BSON document to a PHP variable
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not convert BSON document to a PHP variable
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not convert BSON document to a PHP variable
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not convert BSON document to a PHP variable
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not convert BSON document to a PHP variable
+===DONE===

tests/bson/bug0531.phpt

@@ -0,0 +1,23 @@
+--TEST--
+PHPC-531: Segfault due to double free by corrupt BSON visitor
+--SKIPIF--
+<?php require __DIR__ . "/../utils/basic-skipif.inc"?>
+--FILE--
+<?php
+
+require_once __DIR__ . "/../utils/basic.inc";
+
+$bson = fromPHP(["hello" => "world"]);
+$bson[4] = 1;
+
+echo throws(function() use ($bson) {
+    toPHP($bson);
+}, 'MongoDB\Driver\Exception\UnexpectedValueException'), "\n";
+
+?>
+===DONE===
+<?php exit(0); ?>
+--EXPECTF--
+OK: Got MongoDB\Driver\Exception\UnexpectedValueException
+Could not convert BSON document to a PHP variable
+===DONE===