THRIFT-4263: Fix use after free bug for thrown exceptions
Client: php
Exceptions thrown through PHPExceptionWrapper are prematurely freed at the end
of the catch block, even though zend_throw_exception_object expects to take
ownership of the value.
Ensure we free return_value in case of exceptions
Patch: Håkon Hitland <hakon.hitland@zedge.net>
Patch: Roy Sindre Norangshol <norangshol@zedge.net>
This closes #1314
diff --git a/lib/php/src/ext/thrift_protocol/php_thrift_protocol7.cpp b/lib/php/src/ext/thrift_protocol/php_thrift_protocol7.cpp
index 6d8b76f..3c6c3db 100644
--- a/lib/php/src/ext/thrift_protocol/php_thrift_protocol7.cpp
+++ b/lib/php/src/ext/thrift_protocol/php_thrift_protocol7.cpp
@@ -995,7 +995,10 @@
transport.flush();
} catch (const PHPExceptionWrapper& ex) {
- zend_throw_exception_object(ex);
+ // ex will be destructed, so copy to a zval that zend_throw_exception_object can take ownership of
+ zval myex;
+ ZVAL_COPY(&myex, ex);
+ zend_throw_exception_object(&myex);
RETURN_NULL();
} catch (const std::exception& ex) {
throw_zend_exception_from_std_exception(ex);
@@ -1053,7 +1056,11 @@
zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false);
binary_deserialize_spec(return_value, transport, Z_ARRVAL_P(spec));
} catch (const PHPExceptionWrapper& ex) {
- zend_throw_exception_object(ex);
+ // ex will be destructed, so copy to a zval that zend_throw_exception_object can ownership of
+ zval myex;
+ ZVAL_COPY(&myex, ex);
+ zval_dtor(return_value);
+ zend_throw_exception_object(&myex);
RETURN_NULL();
} catch (const std::exception& ex) {
throw_zend_exception_from_std_exception(ex);
diff --git a/test/php/TestClient.php b/test/php/TestClient.php
index 76fd935..1591027 100755
--- a/test/php/TestClient.php
+++ b/test/php/TestClient.php
@@ -492,6 +492,25 @@
print_r(' caught xception '.$x->errorCode.': '.$x->message."\n");
}
+// Regression test for THRIFT-4263
+print_r("testBinarySerializer_Deserialize('foo')");
+try {
+ \Thrift\Serializer\TBinarySerializer::deserialize(base64_decode('foo'), \ThriftTest\Xtruct2::class);
+ echo "**FAILED**\n";
+ $exitcode |= ERR_STRUCTS;
+} catch (\Thrift\Exception\TTransportException $happy_exception) {
+ // We expected this due to binary data of base64_decode('foo') is less then 4
+ // bytes and it tries to find thrift version number in the transport by
+ // reading i32() at the beginning. Casting to string validates that
+ // exception is still accessible in memory and not corrupted. Without patch,
+ // PHP will error log that the exception doesn't have any tostring method,
+ // which is a lie due to corrupted memory.
+ for($i=99; $i > 0; $i--) {
+ (string)$happy_exception;
+ }
+ print_r(" SUCCESS\n");
+}
+
/**
* Normal tests done.
*/