Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#52765 closed defect (bug) (fixed)

Fatal error: Maximum execution time exceeded in _upgrade_422_find_genericons_files_in_folder()

Reported by: bobbingwide's profile bobbingwide Owned by: afragen's profile afragen
Milestone: 5.9 Priority: normal
Severity: minor Version: 4.2.2
Component: Upgrade/Install Keywords: has-patch commit
Focuses: Cc:

Description

When _upgrade_422_remove_genericons() was written I don't suppose anyone was developing WordPress plugins or themes using node.

Today, I updated a local install of WordPress from 5.6 to 5.7
It timed out after 319 seconds with

PHP Fatal error: Maximum execution time of 300 seconds exceeded in C:\apache\htdocs\cwiccer\wp-admin\includes\update-core.php on line 1553.

I analysed the problem here

https://github.com/bobbingwide/bobbingwide/issues/11

and recreated the issue.

The logic in _upgrade_422_find_genericons_files_in_folder() was called over 70,000 times. The elapsed time was 322 seconds.

Explanation

In this installation I had the development source for over a dozen block plugins.

Proposed fix
Is it still necessary to call upgrade_422_remove_genericons()?

If not, remove it.
If so, reduce the number of directories to be searched.

Attachments (1)

52765.array_filter-vs-foreach-loop-test.php (695 bytes) - added by SergeyBiryukov 4 years ago.

Download all attachments as: .zip

Change History (18)

#1 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.8

Thanks for the report!

Is it still necessary to call upgrade_422_remove_genericons()?

Just noting that this was a security fix added in [32386], see the WordPress 4.2.2 release post for more details.

So I would say the function is still needed, we should be able to adjust it to skip node_modules directories though.

#2 @bobbingwide
4 years ago

So I would say the function is still needed,

Is the function really needed? If so, then the example.html file could be hidden in a node_modules folder. In which case, not scanning these folders would mean the system would once again be vulnerable.
It's a bit unlikely.

I would suggest the scanning's only needed for sites updating from a version prior to 4.2.2.

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


4 years ago

#4 @afragen
4 years ago

As node_modules shouldn't really be in a plugin release, it seems this is more of a developer issue.

Still I think a timeout isn't optimal.

#5 @afragen
4 years ago

Perhaps https://www.phparch.com/2010/04/putting-glob-to-the-test/ may lend some ideas for improvement.

It seems the issue may be

$dirs = glob( $directory . '*', GLOB_ONLYDIR );

#6 @afragen
4 years ago

Might be as simple as changing to line wp-admin/includes/update-core.php:1553

$dirs = glob( $directory . '*', GLOB_ONLYDIR|GLOB_NOSORT );

@bobbingwide can you test the above?

#7 @bobbingwide
4 years ago

I changed my test routine and ran it again. I realised I'd chosen the wrong development site but let it run.

It took 813 seconds for 175,875 folders.
That's for 177 plugins, of which 17 build blocks.

Today, running it on the original site, it took 360 seconds for 70779 directories.
28 seconds slower for an additional 152 directories.

Perhaps there should be a test for a local development environment.

Personally I've never set WP_ENVIRONMENT_TYPE, so wp_get_environment_type() returns 'production'
but if necessary I would be prepared to set it to 'development'.

Or we could just quit if we find a node_modules folder.
I made a simple change that continued to look in folders which weren't node_modules.

Adding the following into the foreach loop in _upgrade_422_find_genericons_files_in_folder()

if ( 'node_modules' === basename( $dir) ) {

				break;	
			}	

reduced the execution time to 3 seconds for the original environment and 10 for the one with 177 plugins (which also had 44 themes).

That's just about acceptable.

#8 @bobbingwide
4 years ago

One other tiny potential issue. There is a very slim chance that an FSE theme will deliver a template part called example.html which contains <title>Genericons</title>.

This template will work fine but will be deleted on an upgrade.

#9 @desrosj
4 years ago

  • Keywords needs-patch added
  • Milestone changed from 5.8 to Future Release
  • Version set to 4.2.2

I agree with @SergeyBiryukov's suggestion above to exclude node_modules and leave this logic in place. Sites still upgrade from really old versions of WordPress all the time. It's becoming less and less common, but it still happens.

This ticket still needs a patch, and the cutoff for for 5.8 is today, so punting to Future Release. Once a patch is added this can be moved back to a numbered milestone.

This ticket was mentioned in PR #1349 on WordPress/wordpress-develop by afragen.


4 years ago
#10

  • Keywords has-patch added; needs-patch removed

When calling _upgrade_422_find_genericons_files_in_folder(), skip the node_modules directory as this can significantly increase the time required to complete the function.

Trac ticket: https://core.trac.wordpress.org/ticket/52765

#11 @afragen
4 years ago

  • Milestone changed from Future Release to 5.9
  • Owner set to afragen
  • Status changed from new to assigned

@desrosj updated PR added to to exclude node_modules directory from scanning.

This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.


4 years ago

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


4 years ago

#14 @SergeyBiryukov
4 years ago

  • Keywords commit added

#15 @SergeyBiryukov
4 years ago

I was curious to compare the performance of the array_filter() approach vs. adding the same check to the foreach loop below, as suggested in comment:7.

Based on a quick test with 100 iterations, while there is no big difference, the array_filter() approach seems to be slightly faster:

array_filter(): 71.29304 seconds
foreach + strpos(): 72.78852 seconds
foreach + basename(): 74.68676 seconds

So the PR should be good to go.

Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#16 @SergeyBiryukov
4 years ago

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

In 51521:

Upgrade/Install: Skip any node_modules directories when removing Genericons example.html files on update.

This can significantly reduce the time required to complete the process in a development environment.

Follow-up to [32386].

Props bobbingwide, afragen, desrosj, SergeyBiryukov.
Fixes #52765.

Note: See TracTickets for help on using tickets.