Fix for 61285 - SSL connections do not timeout #1038

bbroerman30 · 2015-01-30T14:19:01Z

This is a fix for bug 61285 - SSL connections do not timeout, back-ported from trunk

bbroerman30 · 2015-02-03T20:27:58Z

Hi. Wondering when this would be pulled... also, do you want my 5.6 fix, or do you just want to get it from this one?

rdlowrey · 2015-02-03T20:44:26Z

@bbroerman30 Sorry for the slowness -- I meant to get to this sooner.

There is one issue ... the travis build --with-debug-enabled fails:

/home/travis/build/php/php-src/ext/openssl/xp_ssl.c: In function ‘php_openssl_sockop_read’:
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:185:1: warning: control reaches end of non-void function [-Wreturn-type]
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c: In function ‘php_openssl_sockop_write’:
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:180:1: warning: control reaches end of non-void function [-Wreturn-type]

Seems like we're missing a return somewhere inside php_openssl_sockop_io(). Could you look into this? Once we get it resolved I can go ahead with testing/merging.

Also, there's no need for a separate PR for 5.6 -- I'll just pull in your 5.6 branch when merging.

bbroerman30 · 2015-02-04T13:30:10Z

I think the travis build is incorrect:

static size_t php_openssl_sockop_write(php_stream *stream, const char *buf, size_t count TSRMLS_DC)
{
return php_openssl_sockop_io( 0, stream, buf, count );
}

static size_t php_openssl_sockop_read(php_stream *stream, char *buf, size_t count TSRMLS_DC)
{
return php_openssl_sockop_io( 1, stream, buf, count );
}

and the one return at line 239 of php_openssl_sockop_io() and the one at the end (line 334) both return a value (-1 in the event of the first one, and the
number of bytes transferred (read or written) on the one at the end)

On 2/3/2015 3:44 PM, Daniel Lowrey wrote:

@bbroerman30 https://github.com/bbroerman30 Sorry for the slowness -- I meant to get to this sooner.

There is one issue ... the travis build |--with-debug-enabled| fails https://travis-ci.org/php/php-src/jobs/48898871:

|/home/travis/build/php/php-src/ext/openssl/xp_ssl.c: In function ‘php_openssl_sockop_read’:
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:185:1: warning: control reaches end of non-void function [-Wreturn-type]
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c: In function ‘php_openssl_sockop_write’:
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:180:1: warning: control reaches end of non-void function [-Wreturn-type]
|

Seems like we're missing a |return| somewhere inside |php_openssl_sockop_io()|. Could you look into this? Once we get it resolved I can go ahead with
testing/merging.

Also, there's no need for a separate PR for 5.6 -- I'll just pull in your 5.6 branch when merging.

—
Reply to this email directly or view it on GitHub #1038 (comment).

php-pulls · 2015-02-04T14:34:38Z

On Wed, Feb 4, 2015 at 2:30 PM, Brad Broerman notifications@github.com
wrote:

I think the travis build is incorrect:

static size_t php_openssl_sockop_write(php_stream *stream, const char
*buf, size_t count TSRMLS_DC)
{
return php_openssl_sockop_io( 0, stream, buf, count );
}

static size_t php_openssl_sockop_read(php_stream *stream, char *buf,
size_t count TSRMLS_DC)
{
return php_openssl_sockop_io( 1, stream, buf, count );
}

and the one return at line 239 of php_openssl_sockop_io() and the one at
the end (line 334) both return a value (-1 in the event of the first one,
and the
number of bytes transferred (read or written) on the one at the end)

On 2/3/2015 3:44 PM, Daniel Lowrey wrote:

@bbroerman30 https://github.com/bbroerman30 Sorry for the slowness --
I meant to get to this sooner.

There is one issue ... the travis build |--with-debug-enabled| fails <
https://travis-ci.org/php/php-src/jobs/48898871>:

|/home/travis/build/php/php-src/ext/openssl/xp_ssl.c: In function
‘php_openssl_sockop_read’:
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:185:1: warning:
control reaches end of non-void function [-Wreturn-type]
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c: In function
‘php_openssl_sockop_write’:
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:180:1: warning:
control reaches end of non-void function [-Wreturn-type]
|

Seems like we're missing a |return| somewhere inside
|php_openssl_sockop_io()|. Could you look into this? Once we get it
resolved I can go ahead with
testing/merging.

Also, there's no need for a separate PR for 5.6 -- I'll just pull in
your 5.6 branch when merging.

—
Reply to this email directly or view it on GitHub <
https://github.com/php/php-src/pull/1038#issuecomment-72730698>.

—
Reply to this email directly or view it on GitHub
#1038 (comment).

it seems to me that TSRMLS_DC is missing from
https://github.com/bbroerman30/php-src/blob/PHP-5.5/ext/openssl/xp_ssl.c#L179
and
https://github.com/bbroerman30/php-src/blob/PHP-5.5/ext/openssl/xp_ssl.c#L184
which causes the build to bail with

/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:179:2: error: too few
arguments to function ‘php_openssl_sockop_io’
...
/home/travis/build/php/php-src/ext/openssl/xp_ssl.c:184:2: error: too few
arguments to function ‘php_openssl_sockop_io’

Ferenc Kovács
@tyr43l - http://tyrael.hu

bbroerman30 · 2015-02-04T15:08:37Z

Hmm. those weren't included in the trunk build, and that worked. Also, I'm not using any of the PHP internal globals, so I didn't think those were needed... I'll add them in.

Added TSRMLS_CC to php_openssl_sockop_io calls.

Tyrael · 2015-02-04T15:15:52Z

duh, I shouldn't reply via mail, or at least should remove the quoted message.
so yeah, the TSRM macro magic is gone with the master branch, but still in present for 5.5/5.6

bbroerman30 · 2015-02-04T17:57:57Z

@Tyrael made the change, but I don't have access to my test system here at the office. I'll run a test when i get home if need be. I looked at the output from the travis build, and didn't see anything related to this extension in the output, so that's a good sign.

rdlowrey · 2015-02-04T22:03:23Z

@bbroerman30 yeah looks to be fixed. Will test later this evening in win/nix.

bbroerman30 · 2015-02-05T18:38:03Z

@rdlowrey Ran the test suite against openssl in Ubuntu 14.04 and all passed.
Will set up windows env. tonight and run there.

bbroerman30 · 2015-02-06T03:12:55Z

Ran the windows tests, and all succeeded.

rdlowrey · 2015-02-06T06:39:08Z

Thanks @bbroerman30, I just got the same results with the 5.5 PR here. Will pull in your 5.6 changes tomorrow and test those and should be good to go to merge it all up nice and clean :)

rdlowrey · 2015-02-09T17:56:39Z

I have the 5.5/5.6 branches merged locally but currently some ext/openssl tests are failing in master and I'm trying to find out where these errors were introduced. I don't believe it's related to this PR, but I just wanted to give you an update. Once I locate/fix the issue in master I'll finish merging this ...

bbroerman30 · 2015-02-09T18:00:14Z

Thanks... Let me know if I can help.

rdlowrey · 2015-02-09T20:06:24Z

Okay, it seems this is not an "us" problem. The relevant ext/openssl tests work here then start failing in master with the next commit made earlier today ... that can be resolved external to this fix :)

I'll go ahead and merge this upstream in a little while.

dhjw · 2015-03-07T18:43:40Z

I followed the first method here to patch 5.5.9+dfsg-1ubuntu4.6 on Ubuntu Trusty 14.04 with all 3 commits, but it still doesn't time out with a test script like this. Am I doing something wrong? I know the patches are being applied correctly. Here's my full xp_ssl.c and an strace of test.php run via CLI

rdlowrey · 2015-03-07T20:22:51Z

@dhjw I'll see if I can reproduce and get back to you later this weekend

rdlowrey · 2015-03-07T22:05:51Z

@dhjw It seems you're correct :)

There were a few problems which led to the issue you've observed:

1. It appears crypto streams have never actually supported returning appropriate data when stream_get_meta_data() asked for it ... This meant that even if a crypto stream timed out the $meta["timed_out"] boolean would not reflect the state appropriately.

~~2. Crypto streams have never actually supported the assignment of timeouts via stream_set_timeout().~~

The recent implementation changes would eventually timeout the crypto stream in your test script but it was using the connect timeout to do so.

I've corrected each of these problems locally and your reproduce script is now working as expected. I'll update this thread with a link to the appropriate commit once those changes make it up to git and try to get these into the next set of 5.5/5.6 point releases.

Thanks for the report 👍

bbroerman30 · 2015-03-08T00:38:38Z

Ahh did I miss something?

dhjw · 2015-03-08T01:42:09Z

Thank you guys for all your efforts.

rdlowrey · 2015-03-08T02:52:16Z

@bbroerman30 nah your work was fine (and very much appreciated). The issue was just that crypto streams were never originally setup to work with stream_get_meta_data() and stream_set_timeout() (which @dhjw's test script relied upon). All good now, got it sorted 👍

bbroerman30 added 2 commits January 28, 2015 00:04

Updated with SSL fixes (backported from trunk)

fd46416

reneg and should_close are not yet members of sslsock. Removing...

1482ed2

bbroerman30 referenced this pull request Jan 30, 2015

NEWS

ec62b8d

bbroerman30 mentioned this pull request Jan 30, 2015

Fix 61285 - SSL connections do not timeout #971

Merged

smalyshev added the Bug label Feb 1, 2015

Update xp_ssl.c

dddbe0f

Added TSRMLS_CC to php_openssl_sockop_io calls.

php-pulls merged commit dddbe0f into php:PHP-5.5 Feb 9, 2015

miken32 mentioned this pull request Sep 18, 2025

Update stream_get_meta_data return phpstan/phpstan-src#4327

Merged

Fix for 61285 - SSL connections do not timeout #1038

Fix for 61285 - SSL connections do not timeout #1038

Uh oh!

Conversation

bbroerman30 commented Jan 30, 2015

Uh oh!

bbroerman30 commented Feb 3, 2015

Uh oh!

rdlowrey commented Feb 3, 2015

Uh oh!

bbroerman30 commented Feb 4, 2015

Uh oh!

php-pulls commented Feb 4, 2015

Uh oh!

bbroerman30 commented Feb 4, 2015

Uh oh!

Tyrael commented Feb 4, 2015

Uh oh!

bbroerman30 commented Feb 4, 2015

Uh oh!

rdlowrey commented Feb 4, 2015

Uh oh!

bbroerman30 commented Feb 5, 2015

Uh oh!

bbroerman30 commented Feb 6, 2015

Uh oh!

rdlowrey commented Feb 6, 2015

Uh oh!

rdlowrey commented Feb 9, 2015

Uh oh!

bbroerman30 commented Feb 9, 2015

Uh oh!

rdlowrey commented Feb 9, 2015

Uh oh!

dhjw commented Mar 7, 2015

Uh oh!

rdlowrey commented Mar 7, 2015

Uh oh!

rdlowrey commented Mar 7, 2015

Uh oh!

bbroerman30 commented Mar 8, 2015

Uh oh!

dhjw commented Mar 8, 2015

Uh oh!

rdlowrey commented Mar 8, 2015

Uh oh!

Uh oh!