#52765 closed defect (bug) (fixed)
Fatal error: Maximum execution time exceeded in _upgrade_422_find_genericons_files_in_folder()
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (18)
#2
@
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
@
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
@
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
@
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
@
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
@
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
@
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
@
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
#15
@
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.
hellofromtonya commented on PR #1349:
3 years ago
#17
Committed via https://core.trac.wordpress.org/changeset/51521.
Thanks for the report!
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.