Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-4373

Extending Thrift class results in "Attempt serialize from non-Thrift object"

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.10.0
    • 0.12.0
    • PHP - Library
    • None
    • Linux 4.13.3-1-ARCH
      PHP 7.1.10

    • Patch Available
    • Patch

    Description

      This happens when using php extension. thrift_protocol_write_binary will check Z_TYPE_P of spec and expects to be array (IS_ARRAY). However, PHP7 will set it as reference (IS_REFERENCE).

      Example:

      $s = new Serializer\TBinarySerializer();
      
      // Foo is a Thrift type class
      class FooExtended extends Foo {}
      
      $o = new Foo();
      $o2 = new FooExtended();
      
      // this works just fine:
      $s->serialize($o); // this uses thrift_protocol_write_binary if available
      
      // Next line throws \Thrift\Exception\TProtocolException if thrift_protocol_write_binary is used
      // However, it doesn't break if no extension is available.
      $s->serialize($o);
      

      Proposed solution is to dereference using ZVAL_DEREF before checking types (attached). Alternative would be to mark struct type classes as final, but that break compatibility.

      Attachments

        Activity

          githubbot ASF GitHub Bot added a comment -

          GitHub user sokac opened a pull request:

          https://github.com/apache/thrift/pull/1401

          THRIFT-4373: Derefer PHP zval _TSPEC

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/sokac/thrift php

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/thrift/pull/1401.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #1401


          commit d6d493d0f0dcae2f892fe0f5ed99884689069e13
          Author: Josip Sokcevic <jsokcevic@thumbtack.com>
          Date: 2017-10-25T16:45:16Z

          THRIFT-4373: Derefer PHP zval _TSPEC


          githubbot ASF GitHub Bot added a comment - GitHub user sokac opened a pull request: https://github.com/apache/thrift/pull/1401 THRIFT-4373 : Derefer PHP zval _TSPEC You can merge this pull request into a Git repository by running: $ git pull https://github.com/sokac/thrift php Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1401.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1401 commit d6d493d0f0dcae2f892fe0f5ed99884689069e13 Author: Josip Sokcevic <jsokcevic@thumbtack.com> Date: 2017-10-25T16:45:16Z THRIFT-4373 : Derefer PHP zval _TSPEC
          githubbot ASF GitHub Bot added a comment -

          Github user RobberPhex commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1401#discussion_r147051880

          — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp —
          @@ -537,6 +537,9 @@ void binary_deserialize(int8_t thrift_typeID, PHPInputTransport& transport, zval
          }

          zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false);
          + if (spec) {
          — End diff –

          After `zend_read_static_property` with `slient=false`, we should detect exception.
          If no exception, we needn't detect spec is null.

          but we need detect that `Z_TYPE_P(prop) == IS_REFERENCE`

          githubbot ASF GitHub Bot added a comment - Github user RobberPhex commented on a diff in the pull request: https://github.com/apache/thrift/pull/1401#discussion_r147051880 — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp — @@ -537,6 +537,9 @@ void binary_deserialize(int8_t thrift_typeID, PHPInputTransport& transport, zval } zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false); + if (spec) { — End diff – After `zend_read_static_property` with `slient=false`, we should detect exception. If no exception, we needn't detect spec is null. but we need detect that `Z_TYPE_P(prop) == IS_REFERENCE`
          githubbot ASF GitHub Bot added a comment -

          Github user RobberPhex commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1401#discussion_r147052024

          — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp —
          @@ -699,6 +702,9 @@ void binary_serialize_hashtable_key(int8_t keytype, PHPOutputTransport& transpor

          static
          void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* value, HashTable* fieldspec) {
          + if (value) {
          — End diff –

          `Z_TYPE_P(prop) == IS_REFERENCE`

          githubbot ASF GitHub Bot added a comment - Github user RobberPhex commented on a diff in the pull request: https://github.com/apache/thrift/pull/1401#discussion_r147052024 — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp — @@ -699,6 +702,9 @@ void binary_serialize_hashtable_key(int8_t keytype, PHPOutputTransport& transpor static void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* value, HashTable* fieldspec) { + if (value) { — End diff – `Z_TYPE_P(prop) == IS_REFERENCE`
          githubbot ASF GitHub Bot added a comment -

          Github user RobberPhex commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1401#discussion_r147052177

          — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp —
          @@ -709,6 +715,9 @@ void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval*
          throw_tprotocolexception("Attempt to send non-object type as a T_STRUCT", INVALID_DATA);
          }
          zval* spec = zend_read_static_property(Z_OBJCE_P(value), "_TSPEC", sizeof("_TSPEC")-1, true);
          + if (spec) {
          — End diff –

          I think `Z_TYPE_P(prop) == IS_REFERENCE`

          githubbot ASF GitHub Bot added a comment - Github user RobberPhex commented on a diff in the pull request: https://github.com/apache/thrift/pull/1401#discussion_r147052177 — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp — @@ -709,6 +715,9 @@ void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* throw_tprotocolexception("Attempt to send non-object type as a T_STRUCT", INVALID_DATA); } zval* spec = zend_read_static_property(Z_OBJCE_P(value), "_TSPEC", sizeof("_TSPEC")-1, true); + if (spec) { — End diff – I think `Z_TYPE_P(prop) == IS_REFERENCE`
          githubbot ASF GitHub Bot added a comment -

          Github user RobberPhex commented on the issue:

          https://github.com/apache/thrift/pull/1401

          If we want process REFERENCE everywhere.
          I think we should review every line of `zval` used, is it?

          githubbot ASF GitHub Bot added a comment - Github user RobberPhex commented on the issue: https://github.com/apache/thrift/pull/1401 If we want process REFERENCE everywhere. I think we should review every line of `zval` used, is it?
          githubbot ASF GitHub Bot added a comment -

          Github user sokac commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1401#discussion_r147295629

          — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp —
          @@ -537,6 +537,9 @@ void binary_deserialize(int8_t thrift_typeID, PHPInputTransport& transport, zval
          }

          zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false);
          + if (spec) {
          — End diff –

          ZVAL_DEREF checks is reference:
          https://github.com/php/php-src/blob/ffc734b2a455e2f2748f18a54ab597f7e0c715ba/Zend/zend_types.h#L944

          I removed if statement

          githubbot ASF GitHub Bot added a comment - Github user sokac commented on a diff in the pull request: https://github.com/apache/thrift/pull/1401#discussion_r147295629 — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp — @@ -537,6 +537,9 @@ void binary_deserialize(int8_t thrift_typeID, PHPInputTransport& transport, zval } zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false); + if (spec) { — End diff – ZVAL_DEREF checks is reference: https://github.com/php/php-src/blob/ffc734b2a455e2f2748f18a54ab597f7e0c715ba/Zend/zend_types.h#L944 I removed if statement
          githubbot ASF GitHub Bot added a comment -

          Github user sokac commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1401#discussion_r147295660

          — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp —
          @@ -699,6 +702,9 @@ void binary_serialize_hashtable_key(int8_t keytype, PHPOutputTransport& transpor

          static
          void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* value, HashTable* fieldspec) {
          + if (value) {
          — End diff –

          removed if statement

          githubbot ASF GitHub Bot added a comment - Github user sokac commented on a diff in the pull request: https://github.com/apache/thrift/pull/1401#discussion_r147295660 — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp — @@ -699,6 +702,9 @@ void binary_serialize_hashtable_key(int8_t keytype, PHPOutputTransport& transpor static void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* value, HashTable* fieldspec) { + if (value) { — End diff – removed if statement
          githubbot ASF GitHub Bot added a comment -

          Github user sokac commented on a diff in the pull request:

          https://github.com/apache/thrift/pull/1401#discussion_r147295690

          — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp —
          @@ -709,6 +715,9 @@ void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval*
          throw_tprotocolexception("Attempt to send non-object type as a T_STRUCT", INVALID_DATA);
          }
          zval* spec = zend_read_static_property(Z_OBJCE_P(value), "_TSPEC", sizeof("_TSPEC")-1, true);
          + if (spec) {
          — End diff –

          removed if statement

          githubbot ASF GitHub Bot added a comment - Github user sokac commented on a diff in the pull request: https://github.com/apache/thrift/pull/1401#discussion_r147295690 — Diff: lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp — @@ -709,6 +715,9 @@ void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* throw_tprotocolexception("Attempt to send non-object type as a T_STRUCT", INVALID_DATA); } zval* spec = zend_read_static_property(Z_OBJCE_P(value), "_TSPEC", sizeof("_TSPEC")-1, true); + if (spec) { — End diff – removed if statement
          githubbot ASF GitHub Bot added a comment -

          Github user sokac commented on the issue:

          https://github.com/apache/thrift/pull/1401

          @RobberPhex how does this look now? I don't have lots of context for other zval values so I'd leave that for another review.

          githubbot ASF GitHub Bot added a comment - Github user sokac commented on the issue: https://github.com/apache/thrift/pull/1401 @RobberPhex how does this look now? I don't have lots of context for other zval values so I'd leave that for another review.
          githubbot ASF GitHub Bot added a comment -

          Github user james-johnston-thumbtack commented on the issue:

          https://github.com/apache/thrift/pull/1401

          @RobberPhex Any word on getting this merged? Using the extension in PHP 7.0 is kinda broken otherwise... this is fixing a common issue that affected many PHP extensions moving to PHP 7.0.

          githubbot ASF GitHub Bot added a comment - Github user james-johnston-thumbtack commented on the issue: https://github.com/apache/thrift/pull/1401 @RobberPhex Any word on getting this merged? Using the extension in PHP 7.0 is kinda broken otherwise... this is fixing a common issue that affected many PHP extensions moving to PHP 7.0.
          githubbot ASF GitHub Bot added a comment -

          Github user jeking3 commented on the issue:

          https://github.com/apache/thrift/pull/1401

          Need to see if pass the CI build - please squash to a single commit, rebase on master, and force push.

          githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1401 Need to see if pass the CI build - please squash to a single commit, rebase on master, and force push.
          githubbot ASF GitHub Bot added a comment -

          Github user sokac commented on the issue:

          https://github.com/apache/thrift/pull/1401

          @jeking3 squashed and rebased. Looks like CI has some troubles, though

          githubbot ASF GitHub Bot added a comment - Github user sokac commented on the issue: https://github.com/apache/thrift/pull/1401 @jeking3 squashed and rebased. Looks like CI has some troubles, though
          githubbot ASF GitHub Bot added a comment -

          jeking3 closed pull request #1401: THRIFT-4373: Derefer PHP zval _TSPEC
          URL: https://github.com/apache/thrift/pull/1401

          This is a PR merged from a forked repository.
          As GitHub hides the original diff on merge, it is displayed below for
          the sake of provenance:

          As this is a foreign pull request (from a fork), the diff is supplied
          below (as it won't show otherwise due to GitHub magic):

          diff --git a/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp b/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp
          index 75ef2ec8f8..5ac557ffd7 100644
          — a/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp
          +++ b/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp
          @@ -537,6 +537,7 @@ void binary_deserialize(int8_t thrift_typeID, PHPInputTransport& transport, zval
          }

          zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false);
          + ZVAL_DEREF(spec);
          if (EG(exception)) {
          zend_object *ex = EG(exception);
          EG(exception) = nullptr;
          @@ -699,6 +700,9 @@ void binary_serialize_hashtable_key(int8_t keytype, PHPOutputTransport& transpor

          static
          void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* value, HashTable* fieldspec) {
          + if (value)

          { + ZVAL_DEREF(value); + }

          // At this point the typeID (and field num, if applicable) should've already been written to the output so all we need to do is write the payload.
          switch (thrift_typeID)

          { case T_STOP: @@ -709,6 +713,9 @@ void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* throw_tprotocolexception("Attempt to send non-object type as a T_STRUCT", INVALID_DATA); }

          zval* spec = zend_read_static_property(Z_OBJCE_P(value), "_TSPEC", sizeof("_TSPEC")-1, true);
          + if (spec && Z_TYPE_P(spec) == IS_REFERENCE)

          { + ZVAL_DEREF(spec); + }

          if (!spec || Z_TYPE_P(spec) != IS_ARRAY)

          { throw_tprotocolexception("Attempt to send non-Thrift object as a T_STRUCT", INVALID_DATA); }

          @@ -893,7 +900,13 @@ static
          void validate_thrift_object(zval* object) {
          zend_class_entry* object_class_entry = Z_OBJCE_P(object);
          zval* is_validate = zend_read_static_property(object_class_entry, "isValidate", sizeof("isValidate")-1, true);
          + if (is_validate)

          { + ZVAL_DEREF(is_validate); + }

          zval* spec = zend_read_static_property(object_class_entry, "_TSPEC", sizeof("_TSPEC")-1, true);
          + if (spec)

          { + ZVAL_DEREF(spec); + }
          HashPosition key_ptr;
          zval* val_ptr;

          @@ -1027,6 +1040,9 @@ PHP_FUNCTION(thrift_protocol_write_binary) {

          try {
          zval* spec = zend_read_static_property(Z_OBJCE_P(request_struct), "_TSPEC", sizeof("_TSPEC")-1, true);
          + if (spec) {+ ZVAL_DEREF(spec);+ }

          if (!spec || Z_TYPE_P(spec) != IS_ARRAY) {
          throw_tprotocolexception("Attempt serialize from non-Thrift object", INVALID_DATA);
          @@ -1091,6 +1107,7 @@ PHP_FUNCTION(thrift_protocol_read_binary) {
          zval ex;
          createObject("\\Thrift\\Exception
          TApplicationException", &ex);
          zval* spec = zend_read_static_property(Z_OBJCE(ex), "_TSPEC", sizeof("_TPSEC")-1, false);
          + ZVAL_DEREF(spec);
          if (EG(exception)) {
          zend_object *ex = EG(exception);
          EG(exception) = nullptr;
          @@ -1102,6 +1119,9 @@ PHP_FUNCTION(thrift_protocol_read_binary) {

          createObject(ZSTR_VAL(obj_typename), return_value);
          zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, true);
          + if (spec)

          { + ZVAL_DEREF(spec); + }

          if (!spec || Z_TYPE_P(spec) != IS_ARRAY)

          { throw_tprotocolexception("Attempt deserialize to non-Thrift object", INVALID_DATA); }

          @@ -1135,6 +1155,7 @@ PHP_FUNCTION(thrift_protocol_read_binary_after_message_begin)

          { createObject(ZSTR_VAL(obj_typename), return_value); zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false); + ZVAL_DEREF(spec); binary_deserialize_spec(return_value, transport, Z_ARRVAL_P(spec)); }

          catch (const PHPExceptionWrapper& ex) {
          // ex will be destructed, so copy to a zval that zend_throw_exception_object can take ownership of

          ----------------------------------------------------------------
          This is an automated message from the Apache Git Service.
          To respond to the message, please log on GitHub and use the
          URL above to go to the specific comment.

          For queries about this service, please contact Infrastructure at:
          users@infra.apache.org

          githubbot ASF GitHub Bot added a comment - jeking3 closed pull request #1401: THRIFT-4373 : Derefer PHP zval _TSPEC URL: https://github.com/apache/thrift/pull/1401 This is a PR merged from a forked repository. As GitHub hides the original diff on merge, it is displayed below for the sake of provenance: As this is a foreign pull request (from a fork), the diff is supplied below (as it won't show otherwise due to GitHub magic): diff --git a/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp b/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp index 75ef2ec8f8..5ac557ffd7 100644 — a/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp +++ b/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp @@ -537,6 +537,7 @@ void binary_deserialize(int8_t thrift_typeID, PHPInputTransport& transport, zval } zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false); + ZVAL_DEREF(spec); if (EG(exception)) { zend_object *ex = EG(exception); EG(exception) = nullptr; @@ -699,6 +700,9 @@ void binary_serialize_hashtable_key(int8_t keytype, PHPOutputTransport& transpor static void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* value, HashTable* fieldspec) { + if (value) { + ZVAL_DEREF(value); + } // At this point the typeID (and field num, if applicable) should've already been written to the output so all we need to do is write the payload. switch (thrift_typeID) { case T_STOP: @@ -709,6 +713,9 @@ void binary_serialize(int8_t thrift_typeID, PHPOutputTransport& transport, zval* throw_tprotocolexception("Attempt to send non-object type as a T_STRUCT", INVALID_DATA); } zval* spec = zend_read_static_property(Z_OBJCE_P(value), "_TSPEC", sizeof("_TSPEC")-1, true); + if (spec && Z_TYPE_P(spec) == IS_REFERENCE) { + ZVAL_DEREF(spec); + } if (!spec || Z_TYPE_P(spec) != IS_ARRAY) { throw_tprotocolexception("Attempt to send non-Thrift object as a T_STRUCT", INVALID_DATA); } @@ -893,7 +900,13 @@ static void validate_thrift_object(zval* object) { zend_class_entry* object_class_entry = Z_OBJCE_P(object); zval* is_validate = zend_read_static_property(object_class_entry, "isValidate", sizeof("isValidate")-1, true); + if (is_validate) { + ZVAL_DEREF(is_validate); + } zval* spec = zend_read_static_property(object_class_entry, "_TSPEC", sizeof("_TSPEC")-1, true); + if (spec) { + ZVAL_DEREF(spec); + } HashPosition key_ptr; zval* val_ptr; @@ -1027,6 +1040,9 @@ PHP_FUNCTION(thrift_protocol_write_binary) { try { zval* spec = zend_read_static_property(Z_OBJCE_P(request_struct), "_TSPEC", sizeof("_TSPEC")-1, true); + if (spec) {+ ZVAL_DEREF(spec);+ } if (!spec || Z_TYPE_P(spec) != IS_ARRAY) { throw_tprotocolexception("Attempt serialize from non-Thrift object", INVALID_DATA); @@ -1091,6 +1107,7 @@ PHP_FUNCTION(thrift_protocol_read_binary) { zval ex; createObject("\\Thrift\\Exception TApplicationException", &ex); zval* spec = zend_read_static_property(Z_OBJCE(ex), "_TSPEC", sizeof("_TPSEC")-1, false); + ZVAL_DEREF(spec); if (EG(exception)) { zend_object *ex = EG(exception); EG(exception) = nullptr; @@ -1102,6 +1119,9 @@ PHP_FUNCTION(thrift_protocol_read_binary) { createObject(ZSTR_VAL(obj_typename), return_value); zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, true); + if (spec) { + ZVAL_DEREF(spec); + } if (!spec || Z_TYPE_P(spec) != IS_ARRAY) { throw_tprotocolexception("Attempt deserialize to non-Thrift object", INVALID_DATA); } @@ -1135,6 +1155,7 @@ PHP_FUNCTION(thrift_protocol_read_binary_after_message_begin) { createObject(ZSTR_VAL(obj_typename), return_value); zval* spec = zend_read_static_property(Z_OBJCE_P(return_value), "_TSPEC", sizeof("_TSPEC")-1, false); + ZVAL_DEREF(spec); binary_deserialize_spec(return_value, transport, Z_ARRVAL_P(spec)); } catch (const PHPExceptionWrapper& ex) { // ex will be destructed, so copy to a zval that zend_throw_exception_object can take ownership of ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org

          People

            jking3 James E. King III
            sokac Josip Sokcevic
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: