PHPC-531: Fix double free in corrupt BSON visitor

Freeing the current zval in the corrupt BSON visitor causes a segfault if done for the root document, since phongo_bson_to_zval_ex() expects the calling code to initialize and free that zval on success or error.

That said, we should free when encountering an error via the document or array visitors (i.e. nested context), which would have just been initialized.

Author: jmikola

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,11 @@ 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);
 		}
 	}
 
@@ -691,7 +684,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 +730,11 @@ 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);
 		}
 
 	}
@@ -1385,7 +1383,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-004.phpt

@@ -0,0 +1,39 @@
+--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'])),
+    /* 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
+===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===