[jira] [Updated] (THRIFT-2351) PHP TCompactProtocol has fails to decode messages

["Paul Banks (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/THRIFT-2351?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Paul Banks updated THRIFT-2351:
-------------------------------

    Attachment: THRIFT-2351-fix-php-compact-decoding.patch

Patch for above issues.

I tried to get integration tests to verify this however it seems there are no tests that exercise PHP on the service end and given the trivial nature of this patch I wanted to to submit it without committing several more hours to understand and implement more complete coverage of PHP services in the integration test harness.

I was interested to see that only Java is currently tested with the compact protocol which seems an odd decision - certainly the documentation and many "comparisons" of thirft vs XXX on the web make a big thing about the compact protocol and give it the appearance of a first class citizen and yet it seems no one ever tried to test it for cross-language RPC?

Anyway, hopefully you can see from this patch that these are obvious mistakes and typos and should be included even without the extra effort of making integration test runner able to demonstrate the bug.

Thanks. Let me know if I need to submit this differently.

> PHP TCompactProtocol has fails to decode messages
> -------------------------------------------------
>
>                 Key: THRIFT-2351
>                 URL: https://issues.apache.org/jira/browse/THRIFT-2351
>             Project: Thrift
>          Issue Type: Bug
>          Components: PHP - Library
>    Affects Versions: 0.9.1
>         Environment: Tested under PHP 5.3 and HHVM latest.
>            Reporter: Paul Banks
>              Labels: easyfix
>         Attachments: THRIFT-2351-fix-php-compact-decoding.patch
>
>   Original Estimate: 1h
>  Remaining Estimate: 1h
>
> There are two typos in TCompactProtocol::readMessageBegin that prevent correct decoding of any message. They are on consecutive lines:
> https://github.com/apache/thrift/blob/master/lib/php/lib/Thrift/Protocol/TCompactProtocol.php#L390-L391
> First $seqId does not match case of argument $seqid so this is not set PHP is not case sensitive for class names but IS for variable names.
> Second and more importantly, on the following line $name has the decoded length assigned to it instead of $result. This means the method name "hello" ends up being decoded as "6" (the length of the string plus length prefix) and hence any processor trying to dispatch the event will fail.
> I can submit a patch/pull request but its only 6 chars to change and clearly there are no automated tests for this protocol implementation as that should have been caught before.
> I guess not many other people are using PHP as a thrift service processor with this protocol.



--
This message was sent by Atlassian JIRA
(v6.1.5#6160)