When "sidebars_widgets" option is malformed, an infinite loop occurs

[Viper007Bond]

My sidebars_widgets option is set to this:

a:1:{s:9:"sidebar-1";a:3:{i:0;s:7:"links-2";i:1;s:12:"categories-2";i:2;s:10:"archives-2";}}

This results in an infinite loop between retrieve_widgets() and wp_get_sidebars_widgets().

( ! ) Fatal error: Maximum function nesting level of '100' reached, aborting! in D:\Webserver\htdocs\wordpress-dev\wp-includes\cache.php on line 415
Call Stack
#	Time	Memory	Function	Location
1	0.0010	180696	{main}( )	..\themes.php:0
2	0.0017	279048	require_once( 'D:\Webserver\htdocs\wordpress-dev\wp-admin\admin.php' )	..\themes.php:10
3	0.0019	303424	require_once( 'D:\Webserver\htdocs\wordpress-dev\wp-load.php' )	..\admin.php:30
4	0.0022	318672	require_once( 'D:\Webserver\htdocs\wordpress-dev\wp-config.php' )	..\wp-load.php:29
5	0.0028	418968	require_once( 'D:\Webserver\htdocs\wordpress-dev\wp-settings.php' )	..\wp-config.php:85
6	0.1728	19104776	do_action( )	..\wp-settings.php:303
7	0.1805	19115840	call_user_func_array ( )	..\plugin.php:405
8	0.1805	19115968	wp_widgets_init( )	..\plugin.php:0
9	0.1811	19131040	register_widget( )	..\default-widgets.php:1147
10	0.1811	19131280	WP_Widget_Factory->register( )	..\widgets.php:431
11	0.1811	19132032	WP_Widget_Recent_Comments->__construct( )	..\widgets.php:324
12	0.1812	19133736	is_active_widget( )	..\default-widgets.php:603
13	0.1812	19134792	wp_get_sidebars_widgets( )	..\widgets.php:926
14	0.1817	19138232	retrieve_widgets( )	..\widgets.php:1059
15	0.1829	19126896	wp_get_sidebars_widgets( )	..\widgets.php:1222
16	0.1835	19130216	retrieve_widgets( )	..\widgets.php:1059

The consensus in #wordpress-dev is that a break is missing before case 2 : in wp_get_sidebars_widgets(). I'm not sure I agree, but either way it's a bug.

[Viper007Bond]

At least I'm assuming my option is malformed. Isn't it supposed to have an array_version in it?

[Viper007Bond]

Replying to Viper007Bond:

At least I'm assuming my option is malformed. Isn't it supposed to have an array_version in it?

Yes, here's a valid looking value:

a:7:{s:19:"wp_inactive_widgets";a:0:{}s:9:"sidebar-1";a:1:{i:0;s:10:"calendar-6";}s:9:"sidebar-2";a:0:{}s:9:"sidebar-3";a:0:{}s:9:"sidebar-4";a:0:{}s:9:"sidebar-5";a:0:{}s:13:"array_version";i:3;}

No break should be there because if array_version is 1, then both upgrade routines need to run.

[SergeyBiryukov]

Looks like $sidebars_widgets needs to be declared as global in wp_get_sidebars_widgets(). Otherwise retrieve_widgets(), being called from there, sees $sidebars_widgets as undefined, and calls wp_get_sidebars_widgets() again and again.

The patch also fixes the notice when $sidebars_widgets['wp_inactive_widgets'] is not defined.

[Viper007Bond]

Replying to SergeyBiryukov:

Looks like $sidebars_widgets needs to be declared as global in wp_get_sidebars_widgets(). Otherwise retrieve_widgets(), being called from there, sees $sidebars_widgets as undefined, and calls wp_get_sidebars_widgets() again and again.

Aha, so I was right! (That was my guess in #wordpress-dev)

The patch also fixes the notice when $sidebars_widgets['wp_inactive_widgets'] is not defined.

Excellent. I caught that too but didn't want to open a ticket for this edge case. Two birds, one stone.

+1 to patch.

Oh, and I tracked down where the bad option value was coming from: the Automattic theme I'm using was overwriting the sidebars_widgets value if the sidebar was empty. I'll fix it upstream.

[azaozz]

This ticket seems to touch the "Death: remove (finally) compat functions from the widgets API and cleanup converting of old settings (will require copious unit tests first)" point in the ​3.3 schedule.

IMHO the whole switch ( $sidebars_widgets['array_version'] ) block must go. It was added to aid upgrading to WP 2.8. We still could detect $array_version != 3 and delete the whole thing as incompatible.

[SergeyBiryukov]

Replying to azaozz:

IMHO the whole switch ( $sidebars_widgets['array_version'] ) block must go.

Per IRC discussion, the dropping can be moved to 3.4. Is 18598.patch​ good for 3.3?

[aaroncampbell]

Do we really want to add a work around in 3.3 and push the compat to 3.4? Seems like it makes more sense to remove at least this compat now instead.

[lancewillett]

You could probably close this as a duplicate of #17979, the work for that ticket introduced this issue.

I'm attaching a refreshed patch that also removes an extra wp_get_sidebars_widgets() call from inside retrieve_widgets() -- we don't need that extra check, and it adds another loop.

[SergeyBiryukov]

Replying to lancewillett:

also removes an extra wp_get_sidebars_widgets() call from inside retrieve_widgets()

I guess the current logic requires it, unless I'm missing something.

[lancewillett]

Replying to SergeyBiryukov:

Replying to lancewillett:

also removes an extra wp_get_sidebars_widgets() call from inside retrieve_widgets()

I guess the current logic requires it, unless I'm missing something.

It wasn't in the function until recently, and seems to be redundant. Can also cause a loop by calling wp_get_sidebars_widgets() from retrieve_widgets() -- which could call wp_get_sidebars_widgets() again.

Having the global call for $sidebars_widgets (which your patch fixes) takes care of it.

[ryan]

In [18821]:

• Move the old sidebars_widgets array upgrade code to wp-admin/includes/upgrade.php
• Avoid infinite loop with wp_get_sidebars_widgets()
• Remove two unneeded wp_get_sidebars_widgets() calls
• Remove unused $wp_registered_sidebars variable from wp_get_sidebars_widgets().
• Combine a couple of !empty( $sidebars_widgets ) checks in retrieve_widgets()

Props SergeyBiryukov, lancewillett. fixes #17979 #18598