You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by RobberPhex <gi...@git.apache.org> on 2017/10/13 03:20:50 UTC
[GitHub] thrift pull request #1391: Fix segment fault at thrift_protocol extension
GitHub user RobberPhex opened a pull request:
https://github.com/apache/thrift/pull/1391
Fix segment fault at thrift_protocol extension
* while serialize set,
if type at IDL is scalar, directly use key of array as element.
if type at IDL isn't scalar, directly use value of array as element.
* update license at config.m4
* drop php5 support
* after call call_user_function, detect EG(exception)
* zend_read_static_property without silent, cause a EG(exception).
* when serialize set at extension, pass sub-fieldspec to inner-serialize.(do not pass nullptr)
* when validate_thrift_object, if there is no isValidate, skip validate.
* add thrift_protocol_read_binary_after_message_begin for deserialize without method name.
--
close #1383 #1385
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/RobberPhex/thrift fix-php-ext
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1391.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 #1391
----
commit 5fbac48320a042b1d6b8724a796b313cea0f2085
Author: 董菲 <fe...@anjuke.com>
Date: 2016-11-16T02:32:52Z
support php ext read data after message begin
commit d4e29d51da74ecf01a0f8c3921c2f1ff4f37f2c6
Author: 董菲 <fe...@anjuke.com>
Date: 2016-11-16T02:50:22Z
fix
commit 38cc17b8b3ff3e63c0616a5b98c76640bdedebb7
Author: 董菲 <fe...@anjuke.com>
Date: 2016-11-16T03:53:19Z
fix
commit ee55504b8623414446b78dd6251ad7b1517d1d1e
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-03T19:18:38Z
compiler use thrift_protocol_read_binary_after_message_begin
commit d76995b56abd9acdacf99e0c443e6f24e182ed9b
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-04T16:17:15Z
if is_validate is not defined
if isValidate is unset, and last arg of zend_read_static_property is false,
EG(exception) will be set.
So, set last arg of zend_read_static_property to true, and check is_validate is null
commit a6b669a62eb7d2d5d983ab823ceb6a8b8fa07cf8
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-04T17:38:17Z
fix segment caused by zend_read_static_property
* zend_read_static_property with slient(last arg) false, maybe set EG(exception)
* sometimes, we accept unset static property, so call zend_read_static_property with slient(last arg) true
commit 3d03cd9f77e0b772964ccea6ce8cde16a792b8b8
Author: 董菲 <fe...@anjuke.com>
Date: 2016-11-24T09:42:46Z
fix referenced value error
commit 1531b2c76d353b338cd40fc5b5b7326e92ade692
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-04T15:49:01Z
add exception detection after call user funciton
commit 0433fa63bd40411a01b42888126ade969f6615e5
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-09T11:52:34Z
detect is_scalar while compile
commit d90999173928bdbfbe124002e8833627962a0d16
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-09T11:55:43Z
fix list in set
commit 36e8e36b4db851c31c1f84961d61ac7cc92fdda3
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-11T07:05:31Z
remove tail tab
commit 2d8cf19bc9085d2b30aed9a1c7de99472f1ec6fe
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-12T08:48:21Z
process PHPExceptionWrapper
commit 05b5d6bee3aa3e629b4e799f4106da61f532b72b
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-13T02:58:37Z
remove php5 support
commit 05873ef0afe7702f4a049b2d2a681007bf425167
Author: Robert Lu <ro...@gmail.com>
Date: 2017-10-13T03:03:30Z
update license
----
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1391
I saw cross test pass before, so I think we're good.
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1391
Is this for THRIFT-4353? Can you squash it? Rebase it on master to fix the CI issues.
---
[GitHub] thrift pull request #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1391#discussion_r146107107
--- Diff: lib/php/src/ext/thrift_protocol/config.m4 ---
@@ -1,25 +1,27 @@
-dnl Copyright (C) 2009 Facebook
-dnl Copying and distribution of this file, with or without modification,
-dnl are permitted in any medium without royalty provided the copyright
-dnl notice and this notice are preserved.
+dnl Licensed to the Apache Software Foundation (ASF) under one
--- End diff --
dreiss's copyright statement says, "provided the copyright notice and this notice are preserved.". I don't think we should be removing it. You can move it down to be below the Apache Thrift license, or @jfarrell needs to approve this change.
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1391
I already discussed the possibility of retiring the trusty build environment to reduce project maintenance. Maybw now is the time to do it. That said, I would really like to get input from someone else who uses php regularly. @Jens-G perhaps... to ensure this change is acceptable (removing php5 support). I haven't seen any objections in the developer email list but it hasn't been expressly mentioned.
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1391
@jfarrell I see licensing changes in this pull request, can you review?
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex commented on the issue:
https://github.com/apache/thrift/pull/1391
Hi, about license.
`Copyright (C) 2009 Facebook` at file `lib/php/src/ext/thrift_protocol/config.m4` was add by commit f82aee5087bd62989482f5c532cbd80f97a39b7f.
But before this commit, the license at [`php_thrift_protocol.cpp`](https://github.com/apache/thrift/blob/f82aee5087bd62989482f5c532cbd80f97a39b7f^/lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp), [`php_thrift_protocol.h`](https://github.com/apache/thrift/blob/f82aee5087bd62989482f5c532cbd80f97a39b7f^/lib/php/src/ext/thrift_protocol/php_thrift_protocol.h) was Apache License.
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1391
UBsan build failed due to https://issues.apache.org/jira/browse/THRIFT-2913.
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1391
Hmm, I'm just curious about this - given support for php5 was dropped, any idea why the ubuntu-trusty "make check" job succeeded (because it has PHP 5.5.9 in it)? Is the build job not actually running any unit tests or anything that would parse (and fail, if it really is incompatible) the code?
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex commented on the issue:
https://github.com/apache/thrift/pull/1391
at job `Autotools (Ubuntu Trusty)`, it just run `make check`, so php extension cannot be compiled.
at job `Cross Language Tests`, it needn't php extension.
So, I think there already are some CI problems at php part.
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1391
I posted a message on the developer mailing list about removing php5 support. We'll see what comes of it. php5 is currently still shipping so I have some reservations about removing support for it, but I want to hear if there are objections. If there are, can this fix be modified to deal with both?
---
[GitHub] thrift pull request #1391: Fix segment fault at thrift_protocol extension
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/1391
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex commented on the issue:
https://github.com/apache/thrift/pull/1391
this pr fix [`THRIFT-4356`](https://issues.apache.org/jira/browse/THRIFT-4356) and [`THRIFT-4353`](https://issues.apache.org/jira/browse/THRIFT-4353).
---
And, this work is besed `董菲 <fe...@anjuke.com>`'s work. so I just squashed commits by author(not all-in-one commit).
---
[GitHub] thrift pull request #1391: Fix segment fault at thrift_protocol extension
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1391#discussion_r146125400
--- Diff: lib/php/src/ext/thrift_protocol/config.m4 ---
@@ -1,25 +1,27 @@
-dnl Copyright (C) 2009 Facebook
-dnl Copying and distribution of this file, with or without modification,
-dnl are permitted in any medium without royalty provided the copyright
-dnl notice and this notice are preserved.
+dnl Licensed to the Apache Software Foundation (ASF) under one
--- End diff --
OK, I just add APL declaration to `config.m4`
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex commented on the issue:
https://github.com/apache/thrift/pull/1391
@jeking3
Currently, I just remove PHP5 support at PHP extension(thrift_protocol), PHP library is still support PHP5.
So, I think this PR is good.
---
If PHP5 support for extension is needed, I will add it back.
---
[GitHub] thrift issue #1391: Fix segment fault at thrift_protocol extension
Posted by RobberPhex <gi...@git.apache.org>.
Github user RobberPhex commented on the issue:
https://github.com/apache/thrift/pull/1391
build job 4 failed due to https://issues.apache.org/jira/browse/THRIFT-2913.
And, appveyor build queued.
but, same commit at my repo CI is success: https://ci.appveyor.com/project/RobberPhex/thrift/build/1.0.0-dev.58
---