Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#37731 closed defect (bug) (fixed)

Infinite loop in _wp_json_sanity_check() during plugin install

Reported by: maestrosite's profile maestrosite Owned by: swissspidy's profile swissspidy
Milestone: 4.6.1 Priority: normal
Severity: normal Version: 4.6
Component: Upgrade/Install Keywords: has-patch fixed-major
Focuses: administration Cc:

Description

Good day!

After install plugin and l18n-files

  • on wp-instance with localization
  • in action "upgrader_process_complete"
  • in function "wp_update_plugins"

I have got error 500. Because after call \Language_Pack_Upgrader::bulk_upgrade
in function wp_json_encode put object with recursive references Plugin_Upgrader -

  • Plugin_Upgrader::skin = Plugin_Installer_Skin ->
  • Plugin_Installer_Skin::updrader = Plugin_Upgrader ->
  • Plugin_Upgrader::skin = Plugin_Installer_Skin ->
  • Plugin_Installer_Skin::updrader = Plugin_Upgrader ->
  • etc.

Note: plugin and l18n-files installed correctly.

For example, bbPress

( ! ) Fatal error: Maximum function nesting level of '256' reached, aborting! in /home/user/www/wordpress/hdocs/wp-includes/functions.php on line 2992
Call Stack
#	Time	Memory	Function	Location
1	0.0080	363840	{main}( )	.../update.php:0
2	81.0767	6660136	Plugin_Upgrader->install( ???, ??? )	.../update.php:137
3	81.0809	6664584	WP_Upgrader->run( ??? )	.../class-plugin-upgrader.php:108
4	83.7245	6740016	do_action( ???, ???, ??? )	.../class-wp-upgrader.php:791
5	321.2993	6844360	wp_update_plugins( ??? )	.../plugin.php:524
6	394.8101	6924264	wp_json_encode( ???, ???, ??? )	.../update.php:297
7	394.8102	6924640	_wp_json_sanity_check( ???, ??? )	.../functions.php:2900
8	394.8117	6932008	_wp_json_sanity_check( ???, ??? )	.../functions.php:2955
9	394.8121	6934424	_wp_json_sanity_check( ???, ??? )	.../functions.php:2955
10	394.8132	6941392	_wp_json_sanity_check( ???, ??? )	.../functions.php:2955
11	394.8136	6943808	_wp_json_sanity_check( ???, ??? )	.../functions.php:2955

Attachments (7)

37731.patch (905 bytes) - added by ionutst 8 years ago.
37731.2.patch (816 bytes) - added by ionutst 8 years ago.
37731.3.patch (1.1 KB) - added by ionutst 8 years ago.
37731.diff (1.2 KB) - added by swissspidy 8 years ago.
swapping old and new from 33731.3.patch
37731.4.patch (1.1 KB) - added by ionutst 8 years ago.
37731.5.patch (5.9 KB) - added by gitlost 8 years ago.
Demo patch adding 3 shims (includes 37731.4.patch).
37731.6.patch (1.8 KB) - added by gitlost 8 years ago.
Explicitly specify actions take 0 args when adding them back (includes 37731.4.patch).

Download all attachments as: .zip

Change History (33)

#1 @maestrosite
8 years ago

  • Component changed from Gallery to General

This ticket was mentioned in Slack in #feature-shinyupdates by swissspidy. View the logs.


8 years ago

#3 @Kenshino
8 years ago

  • Keywords dev-feedback added

Getting the max nesting level of '256' as well

2016/08/19 15:49:49 [error] 26513#26513: *79089 FastCGI sent in stderr: "PHP message: PHP Fatal error:  Maximum function nesting level of '256' reached, aborting! in /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php on line 2992
PHP message: PHP Stack trace:
PHP message: PHP   1. {main}() /usr/share/nginx/html/client-name/public_html/wp-admin/admin-ajax.php:0
PHP message: PHP   2. do_action() /usr/share/nginx/html/client-name/public_html/wp-admin/admin-ajax.php:91
PHP message: PHP   3. wp_ajax_install_plugin() /usr/share/nginx/html/client-name/public_html/wp-includes/plugin.php:524
PHP message: PHP   4. Plugin_Upgrader->install() /usr/share/nginx/html/client-name/public_html/wp-admin/includes/ajax-actions.php:3612
PHP message: PHP   5. WP_Upgrader->run() /usr/share/nginx/html/client-name/public_html/wp-admin/includes/class-plugin-upgrader.php:108
PHP message: PHP   6. do_action() /usr/share/nginx/html/client-name/public_html/wp-admin/includes/class-wp-upgrader.php:791
PHP message: PHP   7. wp_update_plugins() /usr/share/nginx/html/client-name/public_html/wp-includes/plugin.php:524
PHP message: PHP   8. wp_json_encode() /usr/share/nginx/html/client-name/public_html/wp-includes/update.php:297
PHP message: PHP   9. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2900
PHP message: PHP  10. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP  11. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP  12. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP  13. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP  14. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP  15. _wp_json_sanity_check() /usr/share/nginx
2016/08/19 15:49:49 [error] 26513#26513: *79089 FastCGI sent in stderr: "client-name/public_html/wp-includes/functions.php:2955

followed by repeats of the sanity_check() call until it inadvertently fails

2016/08/19 15:50:38 [error] 26513#26513: *79089 FastCGI sent in stderr: "wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 205. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 206. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 207. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 208. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 209. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 210. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 211. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 212. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 213. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 214. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 215. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 216. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 217. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 218. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message: PHP 219. _wp_json_sanity_check() /usr/share/nginx/html/client-name/public_html/wp-includes/functions.php:2955
PHP message:

I hit this by doing

  1. Go to Plugin Install Screen
  2. Click button to download and install WP Super Cache
  3. Queue Jetpack as well
  4. Wait for a while
  5. Admin notice displays Internal Server Error
  6. Refresh plugin install screen and WP Super Cache is installed, but Jetpack is not.

#4 @netweb
8 years ago

  • Component changed from General to Plugins

#5 @swissspidy
8 years ago

  • Summary changed from Fatal error: Maximum function nesting level of '256' reached, aborting! in /home/user/www/wordpress/hdocs/wp-includes/functions.php on line 2992 to Infinite loop in _wp_json_sanity_check() during plugin install

So… after a successful install or update, wp_update_plugins()gets called.
That function runs wp_json_encode( $extra_stats ), where $extra_stats is the argument passed to the function.
wp_json_encode() runs _wp_json_sanity_check() and the infinite loop begins. But as it is a recursive function, it has a proper termination condition (decrementing $depth)

Since you guys seem to be able to reproduce this issue, could you try adding a var_dump( $extra_stats ) right before the wp_json_encode()line in wp_update_plugins() (line 297)? Maybe a value is passed by reference or something like that, which makes the function call itself hundreds of times.

There was only one change to wp_json_encode() in 4.6, see [37444]. _wp_json_sanity_check() hasn't been touched.

#6 @ionutst
8 years ago

On line 2887 on function.php it has to be

$args[0] = _wp_json_sanity_check( $args[0] , $depth );

instead of

$args[0] = _wp_json_sanity_check( $data, $depth );

Can somebody that has this bug make the changes and test ? ( i can`t reproduce )

Last edited 8 years ago by swissspidy (previous) (diff)

#7 follow-up: @swissspidy
8 years ago

$args[0] is set to $data just above that line so those should be equal…

#8 in reply to: ↑ 7 @ionutst
8 years ago

Replying to swissspidy:

$args[0] is set to $data just above that line so those should be equal…

@swissspidy help me to understand pls. i`m looking at this file:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/functions.php

As i can see here
$args[0] = _wp_json_prepare_data( $data ) ( line 2888 )

Last edited 8 years ago by swissspidy (previous) (diff)

@ionutst
8 years ago

@ionutst
8 years ago

@ionutst
8 years ago

@swissspidy
8 years ago

swapping old and new from 33731.3.patch

#9 @swissspidy
8 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.6.1

Props to ionutst for working on his first WordPress patch to fix this issue.

As one can see from 37731.diff, after [37444] the JSON data wasn't sanitized anymore before going through _wp_json_sanity_check(). Probably because of false assumptions in #36358, but maybe @rmccue can quickly chime in here.

Adding to the 4.6.1 milestone for now as an infinite loop is a pretty bad error. There are tests for wp_json_encode() but unfortunately they didn't catch it as it depends on server environment and the function's arguments.

@ionutst
8 years ago

#10 follow-up: @swissspidy
8 years ago

  • Keywords dev-feedback removed

@Kenshino @maestrosite Can you please test the latest patch to see if it resolves the issue?

#11 in reply to: ↑ 10 @maestrosite
8 years ago

Replying to swissspidy:

@Kenshino @maestrosite Can you please test the latest patch to see if it resolves the issue?

Fail. Response 500 (for ajax request)

in wp-includes/update.php:297

  • $extra_stats = Plugin_Updater
  • $extra_stats->skin = WP_Ajax_Upgrader_Skin
  • $extra_stats->skin->upgrader = Language_Pack_Upgarder
  • $extra_stats->skin->upgrader->skin = WP_Ajax_Upgrader_Skin
  • $extra_stats->skin->upgrader->skin->upgrader = Language_Pack_Upgarder
  • etc...

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


8 years ago

#13 @gitlost
8 years ago

This is caused by adding as 'upgrader_process_complete' actions various functions that don't expect to be called with the arguments it supplies, in this case wp_update_plugins(), which is given a Plugin_Upgrader object as its first argument ($extra_stats), and as the Plugin_Upgrader object has a reference to a WP_Ajax_Upgrader_Skin object which has a reference to a Language_Pack_Upgrader object which has a reference to the same WP_Ajax_Upgrader_Skin object...

A way around it would be to use shims that accept the 'upgrader_process_complete' arguments and then call the original functions with the right arguments. I'll upload a demo patch (incorporating "37731.4.patch" also) that adds shims for the 3 main culprits wp_version_check(), wp_update_plugins and wp_update_themes().

There's 2 others wp_clean_plugins_cache() and wp_clean_themes_cache() that could properly use shims but as they expect booleans it's not such a big deal.

The remaining function async_upgrade() in Language_Pack_Upgrader has recursion avoidance built in.

The patch is against 4.6 but also applies "hunk-successfully" against trunk.

A way to reproduce the bug is to switch languages to eg French and then install an old version of a plugin eg bbPress and then update it.

@gitlost
8 years ago

Demo patch adding 3 shims (includes 37731.4.patch).

#14 @swissspidy
8 years ago

@gitlost Thanks a ton for looking into this! I feel bad for not looking more closely at the function arguments now.

Does this also happen on older WordPress versions? I can't recall that many changes to the upgraders in 4.6...

Last edited 8 years ago by swissspidy (previous) (diff)

#15 @gitlost
8 years ago

Don't think so, the 3 filters were added here 3 months ago...

#16 @gitlost
8 years ago

Don't think so, the 3 filters were added here 3 months ago...

That's wrong, they were just moved there from "wp-includes/update.php", actually added 3 years ago here.

#17 @gitlost
8 years ago

Ah, see now that the only issue is that Language_Pack_Upgrader::bulk_upgrade() is adding back the 3 actions without specifying zero arguments (didn't know that worked!), so don't need no shim schims, just explicit arguments to add_action(). That makes the patch nice and simple.

This was added in r37687 2 months ago so only affects 4.6.

@gitlost
8 years ago

Explicitly specify actions take 0 args when adding them back (includes 37731.4.patch).

#18 @swissspidy
8 years ago

Thanks for further investigating this.

At this point, I think we should open a new ticket for the wp_json_encode() fixes for 4.7 and only fix the upgrade hooks in 4.6.1.

#19 @swissspidy
8 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

#20 @swissspidy
8 years ago

  • Keywords needs-testing removed

Opened a new ticket for the wp_json_encode() fixes.

#21 @swissspidy
8 years ago

  • Component changed from Plugins to Upgrade/Install

#22 @swissspidy
8 years ago

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

In 38415:

Upgrade/Install: After [37687], fix the number of params passed to the upgrade hooks.

wp_version_check(), wp_update_plugins() and wp_update_themes() are all originally hooked to the upgrader_process_complete action with zero arguments passed to them. Zero arguments should be passed when re-adding them after translation updates, otherwise the sky will fall.

Props ionutst, gitlost.
Fixes #37731.

#23 @swissspidy
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#24 @jeremyfelt
8 years ago

For future reference, [25750] and [27928] specified that these should be passed 0 args. [37570] moved all 3 from wp-includes/update.php to wp-admin/includes/admin-filters.php.

#25 @jeremyfelt
8 years ago

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

In 38475:

Upgrade/Install: After [37687], fix the number of params passed to the upgrade hooks.

wp_version_check(), wp_update_plugins() and wp_update_themes() are all originally hooked to the upgrader_process_complete action with zero arguments passed to them. Zero arguments should be passed when re-adding them after translation updates, otherwise the sky will fall.

Merge of [38415] to the 4.6 branch.

Props ionutst, gitlost, swissspidy.
Fixes #37731.

#26 @swissspidy
8 years ago

#37950 was marked as a duplicate.

Note: See TracTickets for help on using tickets.