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

1 files changed, 9 insertions, 2 deletions

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 6d8b76fe9..3c6c3db8e 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 @@ PHP_FUNCTION(thrift_protocol_write_binary) {
     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 @@ PHP_FUNCTION(thrift_protocol_read_binary) {
     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);