Make WordPress Core

Opened 9 years ago

Last modified 16 months ago

#35517 new defect (bug)

Work around PHP7 php-ssh2 breakage

Reported by: dougal's profile dougal Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords: has-patch dev-feedback
Focuses: administration Cc:

Description

There is an updated php-ssh2 package available for PHP7, but it currently breaks the WordPress updater functionality for class-wp-filesystem-ssh2.php. The root cause seems to be that it has not correctly implemented the PHP stream wrappers for the stat() call, and any dependent functions such as is_file(), is_dir(), file_exists(), etc.

However, the ssh2_sftp_stat() function does work, and we can deduce the other information from it.

I've filed a bug against the php-ssh2 extension (https://bugs.php.net/bug.php?id=71376), but I wondered if using ssh2_sftp_stat() might be better, in general, than depending on the PHP stream wrapper functionality.

Attachments (1)

class-wp-filesystem-ssh2.php.patch (2.3 KB) - added by dougal 9 years ago.
Use ssh2_sftp_stat() instead of relying on PHP stream wrappers and allow_url_fopen

Download all attachments as: .zip

Change History (33)

@dougal
9 years ago

Use ssh2_sftp_stat() instead of relying on PHP stream wrappers and allow_url_fopen

#1 @dougal
9 years ago

  • Keywords has-patch added

#3 follow-up: @dd32
9 years ago

I'm rather torn over this. On one hand I want to fix the bug, however, the bug is quite clearly in the extension as it didn't get ported to PHP7 correctly.

If usage of PHP7 + SSH was higher, I'd be more inclined to fix this, however right now this feels like a wontfix assuming the extension gets updated to work properly.

#4 in reply to: ↑ 3 @dougal
9 years ago

Replying to dd32:

I'm rather torn over this. On one hand I want to fix the bug, however, the bug is quite clearly in the extension as it didn't get ported to PHP7 correctly.

If usage of PHP7 + SSH was higher, I'd be more inclined to fix this, however right now this feels like a wontfix assuming the extension gets updated to work properly.

I can understand that. "Not my circus, not my monkeys". Theoretically, it will be a self-correcting problem. And I waffled over whether to file this as a bug or an enhancement in the first place. But just as food for thought...

Do we have any numbers on how many servers have allow_url_fopen turned off? By using the native libssh2 functions instead of relying on the stream wrappers, it might allow a few more systems to function (though I'd have to actually turn that setting off and do real testing before I fully commit to that idea). I don't know if that would affect enough systems to justify the change, though. Especially since it's been stated that overall usage of the ssh2 filesystem isn't terribly high in the first place?

In any case, my patch is working for my own system for now.

#5 follow-up: @dd32
9 years ago

Do we have any numbers on how many servers have allow_url_fopen turned off?

WordPress.org doesn't collect stats on information such as that, and we could switch most of the class to using the ssh2_sftp_* class of functions instead (which wouldn't be restricted by allow_url_fopen) however I'm not sure the intersection between the two is enough to warrant switching.

I will say, eliminating the usage of stream wrappers in preference for the functions seems like a better reason for change than working around the PHP7 breakage.

#6 in reply to: ↑ 5 ; follow-up: @dougal
9 years ago

Replying to dd32:

I will say, eliminating the usage of stream wrappers in preference for the functions seems like a better reason for change than working around the PHP7 breakage.

Would it be worth it for me to re-cast this ticket as a Feature Request for eliminating stream wrapper usage in the class-wp-filesystem-ssh2 code, and see if I need to update my patch for any other methods?

Or should I just install the SSH SFTP Updater Support plugin and forget about it?

#7 in reply to: ↑ 6 @aberbenni
9 years ago

Or should I just install the SSH SFTP Updater Support plugin and forget about it?

SSH SFTP Updater Support does not pass tests with Wp FS Tester, look here ticket:35946#comment:2

#8 @dd32
9 years ago

#35946 was marked as a duplicate.

#9 @aberbenni
9 years ago

Even with the patch I'm having this:

 PHP Warning:  file_get_contents(ssh2.sftp:///var/www/www.website.com/website/wp-content/uploads/js_composer/custom.css): failed to open stream: operation failed in /var/www/www.website.com/website/wp-admin/includes/class-wp-filesystem-ssh2.php on line 223
 PHP Stack trace:
 PHP   1. {main}() /var/www/www.website.com/website/index.php:0
 PHP   2. require() /var/www/www.website.com/website/index.php:17
 PHP   3. require_once() /var/www/www.website.com/website/wp-blog-header.php:16
 PHP   4. do_action() /var/www/www.website.com/website/wp-includes/template-loader.php:12
 PHP   5. Vc_Base->frontCss() /var/www/www.website.com/website/wp-includes/plugin.php:525
 PHP   6. vc_file_get_contents() /var/www/www.website.com/website/wp-content/plugins/js_composer/include/classes/core/class-vc-base.php:549
 PHP   7. WP_Filesystem_SSH2->get_contents() /var/www/www.website.com/website/wp-content/plugins/js_composer/include/helpers/helpers_factory.php:374
 PHP   8. file_get_contents() /var/www/www.website.com/website/wp-admin/includes/class-wp-filesystem-ssh2.php:223

#10 @dd32
9 years ago

@aberbenni Thanks for reporting back on it!

Unfortunately that error is purely within the land of the SSH2 extension and it's communication with your SSH server, there's literally nothing we can really do (other than silence the error, and possibly retry every time it happens) in WordPress to work around that.

#11 @kirasong
9 years ago

  • Version trunk deleted

#12 @dd32
8 years ago

#37942 was marked as a duplicate.

#13 @dd32
8 years ago

#37942 was marked as a duplicate.

As I've marked another ticket as a duplicate of this, I'd just like to express my personal thoughts here.

I personally don't really want to touch the internals of this transport, due to it's limited use, and how rare it is that it's supported.
Testing this change on older versions of the extension, interactions with ssh servers, and my experience of changing internals of transports has told me that if you fix it for one configuration, it'll break ten others.
If the PHP Extension-supplied bindings are faulty, the extension should be fixed instead (which it is, hopefully).

#14 @jobst
8 years ago

(including @dd32)
I really do not understand that the php-ssh2 extension is not used more often!

When you use the direct update FS_METHOD you have to open the entire file tree to the user of the web-server as writeable, otherwise the FS_METHOD=direct does not work.

The FS_METHOD=ssh2 has made me sleep so much better as I can have two users within the entire tree of a Wordpress installation, where the OWNER is the only one being able to write the entire tree and the web-server user can only write a very limited range (e.g. ./wp-content/[upload|cache|upgrade|wf*] plus some css directories).

The php-ssh2 extension allows me to specify the OWNER of the tree as the updater and thus I have no problems updating. It's so simple to stop a malicious script to overwrite index.php using the php-ssh2 extension.

#15 follow-ups: @stianstr23
8 years ago

Hi,

Compiling ssh2 from latest master at https://github.com/php/pecl-networking-ssh2 fixes this issue.

#16 in reply to: ↑ 15 @jobst
8 years ago

Replying to stianstr23:

Compiling ssh2 from latest master at https://github.com/php/pecl-networking-ssh2 fixes this issue.

Not quite, I tried this and it did not work. While it fixes some of the issues the problem with the different ways of handling resources in PHP (just the number) and SSH2 (#resource 123) still has problems. I simply replaced the calls to standard PHP functions (e.g. exist) with calls from the ssh2 lib - that works.

See #39746.

#17 @andythiel
7 years ago

#40541 was marked as a duplicate.

#18 @andythiel
7 years ago

I understand the reasoning that it is hard to fix and that the library needs to be fixed ...
but usage is an argument that surprises me.

I fear many people (those new to Wordpress who use Ubuntu 16.04 and are not hardcore Admins) fix this issue by granting permissions to www-data and setting FS_METHOD to "direct". At least that is the impression that I got from StackExchange like sites.

For me the fixes in the diff above did not solve the issue.
Deleting and Creating folders does not seem to work. Not sure where to look for exact error logging. It might be the same thing aberbenni mentioned or I messed up with copy&paste which the information below suggests.

I too see no other way than giving up on ssh2 (but have a very bad feeling about it).
Hope this gets resolved soon. +1 regarding priority from my side, for whatever that's worth.

Some probably irrelevant information from the Wp FS Tester with the fixes above (probably should not contain any php code ... so yeah maybe I messed up copying the fixes but I double checked):

Files in folder	
		
Plugin location:	/var/www/firsttechfirst.com/wp-content/plugins/wp-filesystem-tester/wp-filesystem-tester.php (Local)
/var/www/firsttechfirst.com/wp-content/plugins/wp-filesystem-tester/wp-filesystem-tester.php (FTP)
Plugin Locations	
hello.php	/var/www/firsttechfirst.com/wp-content/plugins/	Delete file: /var/www/firsttechfirst.com/wp-content/plugins/hello.php
akismet/akismet.php	/var/www/firsttechfirst.com/wp-content/plugins/akismet/	Delete entire folder: /var/www/firsttechfirst.com/wp-content/plugins/akismet/
File IO Errors	Error: Could not create file /var/www/firsttechfirst.com/wp-content/plugins/super-long-name-not-to-conflict.php on server
Downloading a zip	Downloading http://downloads.wordpress.org/plugin/akismet.zip... Suceeded
Extracting Zip	Suceeded
Zip Contents	
akismet/	0	ok	binary
akismet/akismet.php	2409	ok	<?php
/**
 * @package Akismet
 */
/*
Plugin Name: ...
akismet/class.akismet-admin.php	43023	ok	<?php

class Akismet_Admin {
	const NONCE = 'akism...
akismet/class.akismet-cli.php	2712	ok	<?php

WP_CLI::add_command( 'akismet', 'Akismet_CL...
akismet/class.akismet.php	48848	ok	<?php

class Akismet {
	const API_HOST = 'rest.aki...
akismet/class.akismet-widget.php	2825	ok	<?php
/**
 * @package Akismet
 */
class Akismet_Wi...
akismet/.htaccess	629	ok	binary
akismet/_inc/	0	ok	binary
akismet/_inc/akismet.css	11547	ok	binary
akismet/_inc/akismet.js	6921	ok	binary
akismet/_inc/form.js	700	ok	binary
akismet/_inc/img/	0	ok	binary
akismet/_inc/img/logo-full-2x.png	7570	ok	binary
akismet/index.php	26	ok	<?php
# Silence is golden....
akismet/LICENSE.txt	18092	ok	                    GNU GENERAL PUBLIC LICENSE
   ...
akismet/readme.txt	15033	ok	=== Akismet ===
Contributors: matt, ryan, andy, md...
akismet/views/	0	ok	binary
akismet/views/config.php	10608	ok	<div id="akismet-plugin-container">
	<div class="a...
akismet/views/get.php	608	ok	<form name="akismet_activate" action="https://akis...
akismet/views/notice.php	9148	ok	<?php if ( $type == 'plugin' ) :?>
<div class="upd...
akismet/views/start.php	6757	ok	<div id="akismet-plugin-container">
	<div class="a...
akismet/views/stats.php	744	ok	<div id="akismet-plugin-container">
	<div class="a...
akismet/wrapper.php	6437	ok	<?php

global $wpcom_api_key, $akismet_api_host, $...

#19 @ocean90
7 years ago

#41030 was marked as a duplicate.

#20 @supaiku
7 years ago

For some reason the patch didn't resolve it for me, but I found this sketch work around:
https://wordpress.org/support/topic/php-7-compatibility-71/#post-9363511

The SSH plugin, and then someone suggested one. Is it really a safe work-around? :P

#21 in reply to: ↑ 15 @ondrique
7 years ago

Replying to stianstr23:

Hi,

Compiling ssh2 from latest master at https://github.com/php/pecl-networking-ssh2 fixes this issue.

Yes,

I can confirm that updating to the versions below via PECL worked for me too:

SSH2
extension version 1.1.2
libssh2 version 1.5.0

#22 @mangoo
7 years ago

#41831 was marked as a duplicate.

#23 @ralphm
7 years ago

I'm not entirely sure if this is the same problem, but I got upgrades using ssh2 to work with this patch:

--- a/wp-admin/includes/class-wp-filesystem-ssh2.php
+++ b/wp-admin/includes/class-wp-filesystem-ssh2.php
@@ -185,7 +185,7 @@
 		if ( '/' === $path ) {
 			$path = '/./';
 		}
-		return 'ssh2.sftp://' . $this->sftp_link . '/' . ltrim( $path, '/' );
+		return 'ssh2.sftp://' . intval($this->sftp_link) . '/' . ltrim( $path, '/' );
 	}
 
 	/**

#24 @kareltje63
7 years ago

Solved by @ralphm's patch!
Had WordPress-does-not-update-through-ssh2 problem on ubuntu 14.04, upgraded PHP ssh2 to no avail, then upgraded to Ubuntu 16.04, PHP 7, a slightly better error message (instead of an empty page a 'cannot locate wp-content' message), but still not able to upgrade.

Then I applied the intval() patch for file wp-admin/includes/class-wp-filesystem-ssh2.php which solved the problem for me on both the original ubuntu 14.04 WordPress install and the 16.04 install.

Do not forget to apply the patch again after upgrading, until the patch makes it into a new WordPress release

#25 @2bitcoders
7 years ago

Solved by @ralphm's patch on Ubuntu 14.04.5 LTS. After an apt-get update, the issue started. Did a manual update of WP and the issue remained.

Used his update patch and the update-core.php went from 404's to working again.

#26 @SoenHien
7 years ago

Solved by @ralphm's patch!

Problem started after apt-get update & upgrade to Ubuntu 14.04. Apache2 segment fault when installing/updating wordpress plugin/theme.

Confirmed: intval() patch solved it. THANKS.

#27 @quinncom
6 years ago

It wasn't mentioned yet in this ticket, that the bug also exists in ssh2 for PHP 5.6.x (I'm found it with PHP 5.6.36). Apparently, the fixed extension is not available for PHP 5.6.x, so users of PHP < 7 will continue to have this problem.

#28 @ralphm
6 years ago

Is there any chance of getting my fix in comment:23 merged? Manually patching Wordpress after every update is far from ideal.

#29 @desrosj
5 years ago

Related: #48116.

#30 follow-up: @costdev
16 months ago

  • Keywords dev-feedback added

@dd32 Do you have any thoughts on this given that @ralphm's proposed patch has several reports of success?

#31 in reply to: ↑ 30 @dd32
16 months ago

Replying to costdev:

dd32 Do you have any thoughts on this given that @ralphm's proposed patch has several reports of success?

I don't have any recent experience with this module, I'm still supportive of removing the handler from Core however (Given <5% of sites have the extension, and less than that likely use it)

That being said, using intval() seems ridiculous, this is a PHP bug that shouldn't be still affecting others today, but as it's existed for long enough that this should've been merged 5+ years ago. It's documented as needing intval in this example: https://www.php.net/manual/en/function.ssh2-sftp.php

Last edited 16 months ago by dd32 (previous) (diff)

#32 @dd32
16 months ago

This appears to have been fixed since at least version 1.1.2 (2017-07-23) of the extension.
The latest stable release of the pecl extension works

PHP Extension version libssh2 Without intval() with intval()
8.2.5 1.4 (stable: 2023-04-20) 1.10.0 YES YES
7.4.33 1.4 1.10.0 YES YES
7.2.34 1.4 1.10.0 YES -
7.4.33 1.3 (beta: 2021-03-01 ) 1.10.0 YES -
7.4.33 1.2 (beta: 2019-09-18) 1.10.0 YES -
7.2.34 1.1.2 (alpha: 2017-07-23) 1.10.0 YES -
7.2.34 1.0 (alpha: 2016-06-12) 1.10.0 NO YES

I was unable to test with PHP 7.0 and 5.6 due to issues with pecl+https.
I was unable to test with SSH2 0.13, due to dependency conflicts.

The only "stable" releases of the extension are 1.4 and 0.13, 0.13 was not PHP 7.0+ compatible, so all of the PHP7 builds have been alpha/beta's until 1.4.

I'm not inclined to make this commit, as although it appears to work and doesn't appear to break anything, it's only an issue for extensions that are significantly out-of-date and running specific non-stable releases.

Version 1, edited 16 months ago by dd32 (previous) (next) (diff)
Note: See TracTickets for help on using tickets.