WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#7456 closed defect (bug) (fixed)

Timeout in WP_Http_Streams::request causes Fatal error abort

Reported by: wet Owned by:
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description

WP_Http_Streams::request() tries to open a remote stream with fopen() using the default timeout. The user-supplied value per $r is not applied.

If this connection times out, the resulting $handle ist invalid. Nevertheless $handle is subsequently used as an input to stream_set_timeout().

Attached patch applies a timeout to the fopen() context and catches an invalid $handle before use.

Attachments (4)

http.8524.diff (951 bytes) - added by wet 6 years ago.
http.8544.diff (303 bytes) - added by wet 6 years ago.
4779.r8544.diff (1.5 KB) - added by jacobsantos 6 years ago.
Suppresses errors when WP_DEBUG is not enabled and returns WP_Error when malformed url. Actually, parse_url() shouldn't be used to validate urls, but oh well.
7456.r8546.diff (601 bytes) - added by jacobsantos 6 years ago.
Note to self: Don't create patches when dead tired.

Download all attachments as: .zip

Change History (19)

wet6 years ago

comment:1 jacobsantos6 years ago

Looks good. Need to probably duplicate the error processing to the other transport.

comment:2 wet6 years ago

You might also cast an eye on way these kind of errors are handled upstream. In my informal test case, it throws a bunch of notices on both the frontend and the backend stemming from wp-cron.php et al. and thus forcefully hinders the user to enter the backend.

comment:3 ryan6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

comment:4 wet6 years ago

  • Keywords has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

NB: Timeouts in fopen() now decorate the public facing website with a notice:

Warning: fopen(http://example.com/wp-cron.php?check=deadbeef) [function.fopen]: failed to open stream

Additionally, it effectively establishes a denial-of-service barrier to the backend as the PHP notices are the only output from a visit to http://example.com/wp-admin/ . The second notice is:

Warning: Cannot modify header information - headers already sent by (output started at /www/wp-includes/http.php:659) in /www/wp-includes/pluggable.php on line 770

Short-circuiting WP_Http_Streams::request() was the only way to regain admin access, see attached diff.

wet6 years ago

comment:5 jacobsantos6 years ago

What revision number are you at? I will advise updating to latest trunk r8539 and try again.

You can also write a plugin, which short-circuits the transport.

comment:6 wet6 years ago

I'm at r8544, which is latest AFAIK.

I don't think that writing a plugin fixes a symptom which disables backend access, which in turn would be required to activate said plugin. Anything I'm missing?

comment:7 jacobsantos6 years ago

Yeah, you are right, the fix then is to suppress errors from fopen.

jacobsantos6 years ago

Suppresses errors when WP_DEBUG is not enabled and returns WP_Error when malformed url. Actually, parse_url() shouldn't be used to validate urls, but oh well.

comment:8 jacobsantos6 years ago

Can you try the current patch and see if it works for you? I'm guessing that you don't have HTTP or cURL extensions, since those would be used first and you wouldn't be having problems with CRON. However, hopefully, need to write a plugin, which checks the last time the cron was run or something to test the cron is running correctly.

comment:9 ryan6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [8545]) Suppress fopen errors if WP_DEBUG is not enabled. Props jacobsantos. fixes #7456 see #4779

comment:10 wet6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Doesn't work for me.

Observed behaviour:

# Takes approx. 60 seconds from browsing to http://example.com/wp-admin/ to returning a blank screen.

# Adding define('WP_DEBUG', true), I see

Warning: fopen(http://example.com/wp-cron.php?check=deadbeef) [function.fopen]: failed to open stream: [...] timeout [...]. in /www/wp-includes/http.php on line 672

Fatal error: Maximum execution time of 60 seconds exceeded in /www/wp-includes/http.php on line 672

at http://example.com/wp-admin/. This is an expected (but imho undesired) behaviour, as the recent patch won't use $context as an input to fopen() and thus the previously established timeout from $r['timeout] wouldn't come into effect.

As you suspected, my Apache setup didn't include cURL. I subsequently loaded the cURL extension which remedied the deficiencies. So from my POV, WP_Http_Streams currently exhibits insufficient error handling for setups where a fallback from the higher preferred transports is required.

comment:11 jacobsantos6 years ago

Indeed. This must be fixed or it is going to trip up the majority of users.

jacobsantos6 years ago

Note to self: Don't create patches when dead tired.

comment:12 follow-up: jacobsantos6 years ago

Okay, user error mistake in last patch.

comment:13 in reply to: ↑ 12 ; follow-up: jacobsantos6 years ago

Replying to jacobsantos:

Okay, user error mistake in last patch.

7456.r8546.diff should fix that.

comment:14 in reply to: ↑ 13 wet6 years ago

Replying to jacobsantos:

7456.r8546.diff should fix that.

This patch works for me, even with cURL disabled.

comment:15 ryan6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [8548]) Streams transport fixes from jacobsantos. fixes #7456 see #4779

Note: See TracTickets for help on using tickets.