Repository: thrift
Updated Branches:
  refs/heads/master 20116c6c0 -> ec64f23d2


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.hitl...@zedge.net>
Patch: Roy Sindre Norangshol <norangs...@zedge.net>

This closes #1314


Project: http://git-wip-us.apache.org/repos/asf/thrift/repo
Commit: http://git-wip-us.apache.org/repos/asf/thrift/commit/ec64f23d
Tree: http://git-wip-us.apache.org/repos/asf/thrift/tree/ec64f23d
Diff: http://git-wip-us.apache.org/repos/asf/thrift/diff/ec64f23d

Branch: refs/heads/master
Commit: ec64f23d236d7874e3b28ae86c833f57c7aa3389
Parents: 20116c6
Author: Roy Sindre Norangshol <norangs...@zedge.net>
Authored: Wed Jul 26 20:49:38 2017 +0200
Committer: James E. King, III <jk...@apache.org>
Committed: Sat Aug 12 20:11:26 2017 -0700

----------------------------------------------------------------------
 .../ext/thrift_protocol/php_thrift_protocol7.cpp | 11 +++++++++--
 test/php/TestClient.php                          | 19 +++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/thrift/blob/ec64f23d/lib/php/src/ext/thrift_protocol/php_thrift_protocol7.cpp
----------------------------------------------------------------------
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 @@ 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);

http://git-wip-us.apache.org/repos/asf/thrift/blob/ec64f23d/test/php/TestClient.php
----------------------------------------------------------------------
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 @@ try {
   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.
  */

Reply via email to