You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2020/05/08 06:26:47 UTC
[GitHub] [thrift] theilig opened a new pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
theilig opened a new pull request #2134:
URL: https://github.com/apache/thrift/pull/2134
Client: php
<!-- Explain the changes in the pull request below: -->
When you call fwrite in PHP with a handle that has been closed by peer the function will return 0 bytes written (rather than false to indicate an error).
This causes an infinite loop today because the code assumes 0 only means a temporary failure to write, rather than a permanent one.
This change adds a check to for a closed socket when 0 bytes are written
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ X] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ X] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ X] Did you squash your changes to a single commit? (not required, but preferred)
- [ X] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] Jens-G commented on a change in pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#discussion_r422469774
##########
File path: lib/php/lib/Transport/TSocket.php
##########
@@ -328,7 +328,8 @@ public function write($buf)
if ($writable > 0) {
// write buffer to stream
$written = fwrite($this->handle_, $buf);
- if ($written === -1 || $written === false) {
+ $closed_socket = $written === 0 && feof($this->handle_);
+ if ($written === -1 || $written === false || $closed_socket) {
throw new TTransportException(
Review comment:
$written === false
Not your code, but I always wonder if a language has no boolean operators when I see code like this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] Jens-G edited a comment on pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#issuecomment-626130834
Plus, I found this statement in the comments at
https://www.php.net/manual/en/function.fwrite.php#96951
> After having **problems with fwrite() returning 0 in cases where one would fully expect a return value of false**, I took a look at the source code for php's fwrite() itself. The function will only return false if you pass in invalid arguments. **Any other error, just as a broken pipe or closed connection, will result in a return value of less than strlen($string), in most cases 0.**
If that is true then both the docs and all code relying on it are broken.
So why don't we just check for $written being less/equal 0 instead of `=== -1`) ? Wouldn't that suffice?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] theilig commented on pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
theilig commented on pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#issuecomment-626209869
I saw that comment on php.net and verified it was true (both by looking at the c implementation of php's fwrite, and by writing a simple test.
I believe that checking for written <= 0 would work and would be more robust. If there is a closed duplex socket where data was sent from the peer before closing then fwrite will return 0 (the socket is closed) but feof will return false because there is still data to read.
In the patch we are using locally I didn't go that route for two reasons:
1. I'm don't know enough about the internals of sockets to know if there are some cases where a write of 0 is temporary and a subsequent write would be successful.
2. The implementation of TSocket::read uses the 0 length + feof check so I erred on the side of consistency.
`if ($readable > 0) {
$data = fread($this->handle_, $len);
if ($data === false) {
throw new TTransportException('TSocket: Could not read ' . $len . ' bytes from ' .
$this->host_ . ':' . $this->port_);
} elseif ($data == '' && feof($this->handle_)) {
throw new TTransportException('TSocket read 0 bytes');
}
return $data;
}`
I'm pretty sure that throwing an exception on 0 bytes in both cases without the feof check would be correct. We just decided to be really conservative with our initial change as we make tens of thousands of thrift calls per second.
I can attest that adding the feof check has fixed our issue with stuck php processes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] Jens-G edited a comment on pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
Jens-G edited a comment on pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#issuecomment-626130834
Plus, I found this statement in the comments at
https://www.php.net/manual/en/function.fwrite.php#96951
> After having **problems with fwrite() returning 0 in cases where one would fully expect a return value of false**, I took a look at the source code for php's fwrite() itself. The function will only return false if you pass in invalid arguments. **Any other error, just as a broken pipe or closed connection, will result in a return value of less than strlen($string), in most cases 0.**
If that is true then both the docs and all code relying on it are broken.
So why don't we just check for $written being less/equal 0 instead of `=== -1`? Wouldn't that suffice?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] Jens-G commented on pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
Jens-G commented on pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#issuecomment-626130834
Plus, I found this statement in the comments at
> After having **problems with fwrite() returning 0 in cases where one would fully expect a return value of false**, I took a look at the source code for php's fwrite() itself. The function will only return false if you pass in invalid arguments. **Any other error, just as a broken pipe or closed connection, will result in a return value of less than strlen($string), in most cases 0.**
If that is true then both the docs and all code relying on it are broken.
So why don't we just check for $written being less/equal 0 instead of `=== -1`) ? Wouldn't that suffice?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] Jens-G closed pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
Jens-G closed pull request #2134:
URL: https://github.com/apache/thrift/pull/2134
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] Jens-G commented on a change in pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
Jens-G commented on a change in pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#discussion_r422469774
##########
File path: lib/php/lib/Transport/TSocket.php
##########
@@ -328,7 +328,8 @@ public function write($buf)
if ($writable > 0) {
// write buffer to stream
$written = fwrite($this->handle_, $buf);
- if ($written === -1 || $written === false) {
+ $closed_socket = $written === 0 && feof($this->handle_);
+ if ($written === -1 || $written === false || $closed_socket) {
throw new TTransportException(
Review comment:
$written === false
Not your code, but I always wonder if a language has no boolean operators when I see code like this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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
[GitHub] [thrift] theilig edited a comment on pull request #2134: THRIFT-5199: Fix infinite loop writing to closed TSocket in PHP
Posted by GitBox <gi...@apache.org>.
theilig edited a comment on pull request #2134:
URL: https://github.com/apache/thrift/pull/2134#issuecomment-626209869
I saw that comment on php.net and verified it was true (both by looking at the c implementation of php's fwrite, and by writing a simple test.
I believe that checking for written <= 0 would work and would be more robust. If there is a closed duplex socket where data was sent from the peer before closing then fwrite will return 0 (the socket is closed) but feof will return false because there is still data to read.
In the patch we are using locally I didn't go that route for two reasons:
1. I'm don't know enough about the internals of sockets to know if there are some cases where a write of 0 is temporary and a subsequent write would be successful.
2. The implementation of TSocket::read uses the 0 length + feof check so I erred on the side of consistency.
```
if ($readable > 0) {
$data = fread($this->handle_, $len);
if ($data === false) {
throw new TTransportException('TSocket: Could not read ' . $len . ' bytes from ' .
$this->host_ . ':' . $this->port_);
} elseif ($data == '' && feof($this->handle_)) {
throw new TTransportException('TSocket read 0 bytes');
}
return $data;
}
```
I'm pretty sure that throwing an exception on 0 bytes in both cases without the feof check would be correct. We just decided to be really conservative with our initial change as we make tens of thousands of thrift calls per second.
I can attest that adding the feof check has fixed our issue with stuck php processes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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