WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#18598 closed defect (bug) (fixed)

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

Reported by: Viper007Bond Owned by: ryan
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.3
Component: Widgets Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (2)

18598.patch (971 bytes) - added by SergeyBiryukov 3 years ago.
18598-2.diff (1.3 KB) - added by lancewillett 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 follow-up: Viper007Bond3 years ago

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

comment:2 in reply to: ↑ 1 Viper007Bond3 years ago

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.

SergeyBiryukov3 years ago

comment:3 follow-up: SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.3

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.

comment:4 in reply to: ↑ 3 Viper007Bond3 years ago

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.

Last edited 3 years ago by Viper007Bond (previous) (diff)

comment:5 follow-up: azaozz3 years ago

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.

comment:6 in reply to: ↑ 5 SergeyBiryukov3 years ago

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?

comment:7 aaroncampbell3 years ago

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.

comment:8 follow-up: lancewillett3 years ago

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.

lancewillett3 years ago

comment:9 in reply to: ↑ 8 ; follow-up: SergeyBiryukov3 years ago

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.

comment:10 in reply to: ↑ 9 lancewillett3 years ago

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.

comment:11 ryan3 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.