WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20187 closed defect (bug) (fixed)

PemFTP Failing to Fall Back to Pure PHP Implementation

Reported by: merty Owned by: dd32
Milestone: 3.4 Priority: normal
Severity: normal Version: 2.3.1
Component: External Libraries Keywords: has-patch commit
Focuses: Cc:

Description

In /wp-admin/includes/class-ftp.php on line 902:

if(!@dl($prefix . 'sockets.' . PHP_SHLIB_SUFFIX)) $mod_sockets=FALSE;

This is the if-check PemFTP uses to try loading php_sockets extension if it is not enabled already, and to fall back to pure PHP implementation of PemFTP when it fails to load it.

However, if dl() is not available either because the PHP is running in Safe Mode or PHP is built with ZTS support, this if-check silently fails and does not update the $mod_sockets variable with the value FALSE.

To fix that, we can first check whether dl() is callable or not and if it is not callable, we can immediately set $mod_sockets to FALSE and make PemFTP use the pure PHP implementation instead of the one that uses php_sockets.

We just need to replace the line above, with the following:

if(!is_callable('dl') || !@dl($prefix . 'sockets.' . PHP_SHLIB_SUFFIX)) $mod_sockets=FALSE;

Attachments (2)

20187.patch (532 bytes) - added by merty 3 years ago.
20187.2.patch (596 bytes) - added by kurtpayne 3 years ago.
Alternate patch

Download all attachments as: .zip

Change History (15)

@merty3 years ago

comment:1 @merty3 years ago

  • Keywords has-patch added

comment:2 @merty3 years ago

  • Version changed from 3.3.1 to 3.4

comment:3 @kurtpayne3 years ago

  • Cc kpayne@… added
  • Version changed from 3.4 to 2.3.1

@kurtpayne3 years ago

Alternate patch

comment:4 @kurtpayne3 years ago

I like the idea of checking that dl() is available before calling it. That function could be disabled by the server admin.

When setting the value of $mod_sockets, is there a downside to checking for the existence of the socket functions directly?

$mod_sockets = function_exists( 'socket_set_option' );

comment:5 @merty3 years ago

Technically there is no difference but extension_loaded('sockets') looks like a better coding practice to me.

The actual issue, however, isn't detecting the availability of php_sockets. The issue is, wherever dl() is not available, the if-check silently fails and therefore it does not set $mod_sockets's value to FALSE even though it should.

dl() is deprecated in PHP 5.3 and completely removed in PHP6. Either we can completely remove the part where it tries to load the extension in the runtime, or we can just check the availability of dl() and prevent unexpected behaviors.

Last edited 3 years ago by merty (previous) (diff)

comment:6 @dd323 years ago

. If the functionality of loading modules is not available or has been disabled (either by setting enable_dl off or by enabling safe mode in php.ini) an E_ERROR is emitted and execution is stopped. If dl() fails because the specified library couldn't be loaded, in addition to FALSE an E_WARNING message is emitted.

This makes it sound like the Execution is halted if loading modules is disabled.. which doesn't sound right to me.. btu failing with a true-ey value when it's disabled doesn't sound right either..

As for the code in PemFTP, as it's an external library, anything here should also be raised as a ticket/issue for the library - although i seem to recall that development of PemFTP doesn't have any active tracker.

As for the code here in WordPress, I'm all for changing to making sure it's callable, and checking capabilities rather than the result of dl(). although, I think we should keep to using extension_loaded() as in the if block that surounds that code, rather than function_exists().

For reference, I believe this is the only location in WordPress that we actually try using dl(), We don't do similar things for MySQL or GD (although, they'd benefit more than FTP would from it's usage).

comment:7 @merty3 years ago

Unfortunately PemFTP is no longer being developed by its developer. The last change made on the code is 3.5 years old. I'm thinking of forking it to keep it up-to-date.

Until WordPress completely drops support for PHP5, which is something we are years away I believe, using is_callable() is the best solution I think. It harms no one but may benefit someone.

comment:8 @nacin3 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to 3.4

We should probably commit this. This causes execution to terminate otherwise.

comment:9 @dd323 years ago

For what it's worth, I couldn't cause the script to die.

If dl is disabled via safemode or enable_dl = Off, the function simply returns false. If the function is disabled via disable_functions, it returns null. False or Null would trigger setting it to false.

in the above cases, is_callable() would return true, as it could be called. function_exists() would return false however. This may differ in a binary that it was specifically removed in though, which would cause it to die.

Instead, I'll commit a version which should avoid any issues..

comment:10 @dd323 years ago

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

In [20311]:

WP_Filesystem: PemFTP: Detect dl() being disabled in order to avoid any fatals which may be produced when the Sockets extension is not available. Props to kurtpayne for initial patch, Fixes #20187

comment:11 follow-up: @nacin3 years ago

Per the docs on php.net/dl:

If the functionality of loading modules is not available or has been disabled (either by setting enable_dl off or by enabling safe mode in php.ini) an E_ERROR is emitted and execution is stopped.

I was able to reproduce this even with the error suppression operator.

comment:12 in reply to: ↑ 11 ; follow-up: @dd323 years ago

Replying to nacin:

Per the docs on php.net/dl:

If the functionality of loading modules is not available or has been disabled (either by setting enable_dl off or by enabling safe mode in php.ini) an E_ERROR is emitted and execution is stopped.

I was able to reproduce this even with the error suppression operator.

In both of those cases, for me, an E_ERROR was raised, execution of the dl() operation ceased, and script execution continued, although I'm using php-fpm which may act differently than the apache module.

comment:13 in reply to: ↑ 12 @merty3 years ago

Replying to dd32:

Replying to nacin:

Per the docs on php.net/dl:

If the functionality of loading modules is not available or has been disabled (either by setting enable_dl off or by enabling safe mode in php.ini) an E_ERROR is emitted and execution is stopped.

I was able to reproduce this even with the error suppression operator.

In both of those cases, for me, an E_ERROR was raised, execution of the dl() operation ceased, and script execution continued, although I'm using php-fpm which may act differently than the apache module.

The problem is, it does not update the mod_sockets variable with the value FALSE even when it raises an error. At least, it did not when we tried on three different servers.

Version 1, edited 3 years ago by merty (previous) (next) (diff)
Note: See TracTickets for help on using tickets.