[jira] [Commented] (THRIFT-3984) PHP7 extenstion cause segfault

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/THRIFT-3984?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15854461#comment-15854461 ] 

ASF GitHub Bot commented on THRIFT-3984:
----------------------------------------

Github user jeking3 commented on the issue:

    https://github.com/apache/thrift/pull/1152
  
    Not a php expert but this looks okay to me, however I would like to see it pass CI.  How do we trigger a new build on someone else's pull request?  Otherwise, if the original author could rebase on master, ideally squash the commits to one, and push again that would be helpful and would kick off a new build.


> PHP7 extenstion cause segfault
> ------------------------------
>
>                 Key: THRIFT-3984
>                 URL: https://issues.apache.org/jira/browse/THRIFT-3984
>             Project: Thrift
>          Issue Type: Bug
>          Components: PHP - Library
>    Affects Versions: 0.10.0
>            Reporter: Fei Dong
>            Priority: Critical
>
> I cant offer a script to reproduce segfault because it  not happens everytime. 
> PHP7 extension use ZVAL_STR to wrap zend_string to zval struct，and later use zval_dtor try to free it.
> https://github.com/apache/thrift/blob/master/lib/php/src/ext/thrift_protocol/php_thrift_protocol7.cpp#L825
> The method_name parameter is pass from PHP script,  and zval_dtor decrement the gc reference count, thus would free the zend_string object but it is still referenced in the script.
> I  changed  ZVAL_STR to ZVAL_STR_COPY, which will add reference count by 1 to the zend_string object ,  apply this patch in our production environment and segfault never happen again
> Another place use ZVAL_STR is [here|https://github.com/apache/thrift/blob/master/lib/php/src/ext/thrift_protocol/php_thrift_protocol7.cpp#L668]. Both this two place need to be fixed 



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)