Make WordPress Core

Opened 22 months ago

Closed 13 months ago

Last modified 13 months ago

#55711 closed enhancement (fixed)

Check that set_time_limit() function is available before using it in core

Reported by: theode's profile theode Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.2 Priority: normal
Severity: normal Version:
Component: General Keywords: php8 needs-refresh
Focuses: administration Cc:

Description (last modified by costdev)

Original title: set_time_limit( 300 ); in Class WP_Upgrader causes crashes in PHP8 when hosting disables it

Some hosting providers disable set_time_limit(). That is causing a fatal error when installing plugins, themes and import files under PHP8.

My Suggestion is to prefix them all with an @ or remove them entirely.

Reproducing. Not sure how to deactivate it for PHP8, but is an issue.

Can't find a ticket, already stating this.

Change History (27)

#1 @SergeyBiryukov
22 months ago

Hi there, welcome back to WordPress Trac! Thanks for the ticket.

Just linking to some related tickets here: #19487, #21521, #46292, #49014, #52734.

#2 @theode
22 months ago

so whats the solution, why they keep it?

#3 @johnbillion
22 months ago

I believe that @-suppressing this call will still cause a fatal with PHP 8 because it no longer suppresses fatal errors. If it's common for hosts to disable this function then the calls need to be wrapped with function_exists( 'set_time_limit' ). I know WooCommerce for example wraps this in its wc_set_time_limit() function.

Last edited 22 months ago by johnbillion (previous) (diff)

#4 @hellofromTonya
22 months ago

I agree with @johnbillion. function_exists( 'set_time_limit' ) would guard against invoking the native PHP function when it's been disabled and is not in memory.

The issue though is beyond just the WP_Upgrader class. Core has 10 total instances of using set_time_limit(). Introducing a wp_set_time_limit() wrapper function could encapsulate the code for reuse in each of these instances.

How are other projects handling this?

SMTP and WooCommerce offer slightly different solutions.

  • Both are checking the if 'set_time_limit' is in 'disable_functions' ini list:
false === strpos( ini_get( 'disable_functions' ), 'set_time_limit' )
  • WooCommerce goes further to also check function_exists() and 'safe_mode' (in the ini).
  • SMTP uses the @ though this will be ineffective in >= PHP 8.

Reference:

#5 follow-up: @costdev
22 months ago

  • Keywords php8 added

@hellofromTonya function_exists( 'set_time_limit' ) would guard against invoking the native PHP function when it's been disabled and is not in memory.

To clarify this:

  • function_exists() will return false for a native function that is not in memory or in the PHP binary. However, it will return true for a native function disabled via the disable_functions directive. (I know!)
  • There are three ways I'm aware of to return true for a native function disabled via disabled_functions:
    1. false === strpos() as WooCommerce and SMTP use.
    2. ! str_contains() - a modern version of the above.
    3. is_callable() - Which feels more accurate in context. Check if it exists with function_exists(), check if it's callable with is_callable().

#6 in reply to: ↑ 5 @jrf
22 months ago

Replying to costdev:

@hellofromTonya function_exists( 'set_time_limit' ) would guard against invoking the native PHP function when it's been disabled and is not in memory.

To clarify this:

  • function_exists() will return false for a native function that is not in memory or in the PHP binary. However, it will return true for a native function disabled via the disable_functions directive. (I know!)
  • There are three ways I'm aware of to return true for a native function disabled via disabled_functions:
    1. false === strpos() as WooCommerce and SMTP use.
    2. ! str_contains() - a modern version of the above.
    3. is_callable() - Which feels more accurate in context. Check if it exists with function_exists(), check if it's callable with is_callable().

Well actually....

Only a check with strpos()/str_contains() will give consistent results cross-version, which makes every other way of checking this invalid.

There was a change in how PHP reports on disabled functions in PHP 8.0 which is relevant here:

Disabled functions are now treated exactly like non-existent functions.
Calling a disabled function will report it as unknown, and redefining a disabled function is now possible.

Ref: https://github.com/php/php-src/blob/5e34638c1cc42520c86008cec2f7b20f3d678701/UPGRADING#L186-L188

This can be confirmed by running the following script on multiple PHP versions:

<?php
echo 'function_exists: ', var_export(function_exists('set_time_limit'), true), PHP_EOL;
echo 'is_callable: ', var_export(is_callable('set_time_limit'), true), PHP_EOL;
echo 'strpos/str_contains: ', var_export(strpos( ini_get( 'disable_functions' ), 'set_time_limit' ), true), PHP_EOL;

Do make sure to set disable_functions = 'set_time_limit' in the php.ini file prior to running the script as it can not be changed at run-time (which is also the reason why I'm not posting an 3v4l link).

The output will look like this:

"PHP 5.6"
------------------
function_exists: false
is_callable: true
strpos/str_contains: 0

"PHP 7.0"
------------------
function_exists: false
is_callable: true
strpos/str_contains: 0

"PHP 7.2"
------------------
function_exists: false
is_callable: true
strpos/str_contains: 0

"PHP 7.4"
------------------
function_exists: true
is_callable: true
strpos/str_contains: 0

"PHP 8.0"
------------------
function_exists: false
is_callable: false
strpos/str_contains: 0

"PHP 8.1"
------------------
function_exists: false
is_callable: false
strpos/str_contains: 0

#7 @jrf
22 months ago

Note: what about introducing a wp_is_function_disabled( $function_name ) function (better names welcome) ?

I know there are several places in the code base where function checks are being done as misinformed hosts have a tendency to disable those functions.

Now might be good time to streamline those checks throughout the codebase to ensure the verification of the callability of the potentially disabled functions is done consistently.

#8 follow-up: @costdev
22 months ago

@jrf Well actually... I stand corrected 😂. I spend most of my Core dev time on PHP 7.4, so that explains my results. It's surprising to me that PHP 7.4 returns a different result for function_exists(), but that's a discussion for another forum.

I agree that wrapping these checks in a function would be a great addition to Core!

On the name: I wonder, since we're checking function_exists() and such, would wp_is_function_available(), wp_is_function_callable() or even wp_is_callable() be more appropriate?

Obligatory: naming is hard.

#9 @costdev
22 months ago

  • Component changed from Upgrade/Install to General
  • Description modified (diff)
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.1
  • Severity changed from critical to normal
  • Summary changed from set_time_limit( 300 ); in Class WP_Upgrader causes crashes in PHP8 when hosting disables it to Introduce a function to determine whether a function can be called.
  • Type changed from defect (bug) to task (blessed)
  • Version 5.9.3 deleted

As the arguments in favour of a function to handle all the checks is clear, I think we should handle this in this ticket instead of adding overhead by potentially patching three conditions for this issue and then removing it shortly after.

I'm changing this to a Task and widening the scope so that we can discuss the addition of a new function to handle this.

Last edited 22 months ago by costdev (previous) (diff)

#10 in reply to: ↑ 8 @SergeyBiryukov
22 months ago

Replying to costdev:

@jrf Well actually... I stand corrected 😂. I spend most of my Core dev time on PHP 7.4, so that explains my results. It's surprising to me that PHP 7.4 returns a different result for function_exists(), but that's a discussion for another forum.

Hmm, I could not reproduce that on PHP 7.4.15. This is what the script above returns for me, same as on other PHP 7.x versions:

"PHP 7.4"
------------------
function_exists: false
is_callable: true
strpos/str_contains: 0

As previously noted in comment:3:ticket:42085 and comment:116:ticket:51857, function_exists() can return true for disabled functions when Suhosin is in use, but I'm not aware of any other factors that might cause that:

Per the comments in the PHP manual, function_exists() should return false for functions disabled via disable_functions setting.

As also noted in comment:3:ticket:42085, it is possible for the function_exists() check to return true if Suhosin is in use. However, that would be an edge case, as Suhosin was only officially available for PHP 5.4 to 5.6, and its development was discontinued in 2015.

It's also worth noting that as of PHP 8, disabled functions can be redeclared. Unless the function is redeclared, function_exists() still returns false.

It looks like most of the instances in core where we need to check for a function that might be disabled just use a simple function_exists() check.

Related: #33112, #37680, #37978, #48693, #52226, #54780.

So at the moment I'm not sure we need to do anything besides a function_exists() check here, that seems to be its intended purpose.

Last edited 22 months ago by SergeyBiryukov (previous) (diff)

#11 @davidbaumwald
17 months ago

If this adds a new function in a milestone, this should be an Enhancement.

Anyone strongly against moving this to 6.2? The deadline for FEA/EHN for 6.1 was Beta 1 a couple of days ago.

#12 @davidbaumwald
17 months ago

  • Milestone changed from 6.1 to 6.2
  • Type changed from task (blessed) to enhancement

#13 @SergeyBiryukov
15 months ago

#57248 was marked as a duplicate.

This ticket was mentioned in PR #3716 on WordPress/wordpress-develop by jokerrs.


15 months ago
#14

  • Keywords has-patch added; needs-patch removed

There is a lots of shared hosting servers, which one has disabled set_time_limit function by php.ini. So usually that makes problems with updating and maintaining the WordPress.
I've made a if test for all set_time_limit I've found at the source, so they do not trigger if that function is disabled by php.ini.

<?php
if (strpos(ini_get('disable_functions'), 'set_time_limit') === 0){
    set_time_limit($some_time);
}

Trac ticket: #55711, #57248

@jrf commented on PR #3716:


15 months ago
#15

Please read the thread in https://core.trac.wordpress.org/ticket/55711 - function_exists() is not a good way to do this.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


13 months ago

#17 follow-up: @costdev
13 months ago

  • Keywords needs-patch added; has-patch removed

This ticket was discussed in the recent bug scrub. PR 3716 uses function_exists(), which the discussion in this ticket has determined isn't reliable.

Removing has-patch and adding needs-patch.

Props @mukesh27

This ticket was mentioned in Slack in #core by costdev. View the logs.


13 months ago

#19 @costdev
13 months ago

  • Milestone changed from 6.2 to Future Release

This ticket was discussed during the bug scrub. As a new patch is needed and we're approaching 6.2 Beta 1 in a few days, I'm moving this ticket to Future Release.

Additional props: @davidbaumwald @petitphp @audrasjb

#20 in reply to: ↑ 17 @SergeyBiryukov
13 months ago

Replying to costdev:

This ticket was discussed in the recent bug scrub. PR 3716 uses function_exists(), which the discussion in this ticket has determined isn't reliable.

I might be missing something, but I don't see any follow-up to comment:10 that would indicate anything other than a function_exists() check is needed here. Except for when Suhosin is in use, do we know any other circumstances when this does not work as expected?

#21 follow-up: @SergeyBiryukov
13 months ago

  • Keywords needs-refresh added; needs-patch removed
  • Milestone changed from Future Release to 6.2
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

function_exists() seems to match what we generally use in other places of core to check for potentially disabled functions, e.g. ini_get(), so I'm not quite sure why this ticket is blocked in search of a different solution.

Doing the same for set_time_limit() seems like a good first step here. PR 3716 could use a refresh to exclude external libraries (getID3, PHPMailer), but otherwise looks good to me.

A new ticket could then be opened to explore alternatives to function_exists() if there is evidence of its unreliability, and to replace all of its instances in core.

#22 @SergeyBiryukov
13 months ago

  • Summary changed from Introduce a function to determine whether a function can be called. to Check that set_time_limit() function is available before using it in core

#23 in reply to: ↑ 21 @azaozz
13 months ago

Replying to SergeyBiryukov:

Doing the same for set_time_limit() seems like a good first step here.

+1

Also wondering how many actual cases of disabled set_time_limit() exist "in the wild"? Seems this function has been used in WP for many years. If it was disabled and throwing fatal errors, there would have been a lot of reports about it, right?

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


13 months ago

#25 @SergeyBiryukov
13 months ago

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

In 55258:

General: Check that set_time_limit() function is available before using it in core.

This avoids a fatal error if the function is disabled on certain environments.

Props theode, jokerrs, johnbillion, hellofromTonya, costdev, jrf, azaozz, SergeyBiryukov.
Fixes #55711.

@SergeyBiryukov commented on PR #3716:


13 months ago
#26

Thanks for the PR! Merged in r55258.

As noted in the comments on the ticket, we can follow up later if an alternative to function_exists() should be considered.

#27 @SergeyBiryukov
13 months ago

Looks like I missed the props for @TobiasBg here. Sorry for that!

I have updated the props list for [55258] in the Core Props tool on make/core to correct that.

Note: See TracTickets for help on using tickets.