WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#31147 closed defect (bug) (fixed)

make sure $notoptions is an array before assigning array key value

Reported by: hauvong Owned by: wonderboymusic
Milestone: 4.3 Priority: normal
Severity: normal Version: 2.2
Component: Options, Meta APIs Keywords: has-patch
Focuses: Cc:
PR Number:

Description

In a multi sites worpress setup environment, there are times where get_options would cause illegal string offset warnings.

The issue is caused by $notoptions that can contain a string from get_get_cache call.
To avoid the potential warning errors, we should check to make sure $notoptions variable is an array before trying to assign array indexed value to it.

For reference: https://github.com/Automattic/vip-quickstart/issues/216

Attachments (1)

wp-option.php.patch (959 bytes) - added by hauvong 5 years ago.
Patch file to fix the issue

Download all attachments as: .zip

Change History (17)

@hauvong
5 years ago

Patch file to fix the issue

#1 @SergeyBiryukov
5 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.2
  • Version changed from trunk to 2.2

Introduced in [4855].

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


5 years ago

#3 @ocean90
5 years ago

  • Milestone changed from 4.2 to Awaiting Review

I would be more interested in why wp_cache_get( 'notoptions', 'options' ); returns a string.

@hauvong, do you know what the value of $notoptions is in such a case? @westonruter had mentioned in this comment that it's "{sitename} Sites". Can you confirm that?

If we're going to force $notoptions to be an array it should probably happen before if ( isset( $notoptions[ $option ] ) ) {.

#4 @jipmoors
5 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #28664.

#5 @DrewAPicture
5 years ago

  • Milestone Awaiting Review deleted

#6 @spacedmonkey
5 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Why has this been closed. #28664 seems unrelated to this issue. If have a poisoned cache, meaning you have a string in place of a array in site options, you will get a Illegal string offset error. This is an error in the get_site_option function and not get_option. This is a multisite issue only.

#7 @wonderboymusic
5 years ago

  • Milestone set to Future Release

This happens to us constantly - we hacked core to log these instances in New Relic, happens 1000s of times on a giant multisite install.

I really, really want to figure out the array issue and the cache pollution issue.

#8 @spacedmonkey
5 years ago

The error is now happening for me on a large multisite once a week. It gums up my logs with E_WARNING: Illegal string offset errors. This needs bug needs to be fixed and put into core ASAP. The patch provided seems good to me. What are the next steps to put this into core?

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


5 years ago

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


5 years ago

#11 @spacedmonkey
4 years ago

This bug is happening for me all the time now (twice a week). So here are some more details on my setup.

I am hosting a large scale WP multisite in AWS, over 8 web servers. I have remote memcache servers that all the web servers talk to. It seems that somehow cache value {$wpdb->siteid}:notoptions (group - site-options), is getting set to a string instead of an array. I believe this is happening outside of WordPress and is possibly a issue with AWS. I believe it is a communication issue with memcache, as once (and I can't confirm this as I haven't seen it again) when using WP CLI to review the cache value, I saw http connection header set as the value. As the value is expected to be an array and not a string, the code errors, as it gets an Illegal string offset as the type is wrong. The error that new relic reports, is different everytime, as it is different keys it is trying to access in the array, but the error is effectively the same. It seem to be more likely to siteurl, but I have seen other array key being accessed.

A cache flush / change of cache key clears the issue, but doesn't fix it. The patch provided seem like it will stop all the type errors I am getting in my reporting tools. I don't believe this will fix the problem all together because I am not sure that wordpress is to blame for the issue. Either way, the patch doesn't seem like it would hurt anything to just merge it....

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


4 years ago

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


4 years ago

#14 @spacedmonkey
4 years ago

For reference, the stack trace

Error message
E_WARNING: Illegal string offset 'users_can_register'

in get_site_option called at <path>/web/wp-includes/option.php (990)
…gins/better-wp-security/modules/free/hide-backend/
class-itsec-hide-backend.php (123)
in ITSEC_Hide_Backend::execute_hide_backend called at ? (?)
in call_user_func_array called at <path>/web/wp-includes/plugin.php (505)
in do_action called at <path>/web/wp-settings.php (353)
in require_once called at <path>/wp-config.php (180)
in require_once called at <path>/web/wp-load.php (34)
in require_once called at <path>/web/wp-blog-header.php (12)
in require called at <path>/web/index.php (17)
Error message
E_WARNING: Illegal string offset 'users_can_register'

in get_site_option called at <path>/web/wp-includes/option.php (990)
…inspirewp/web/wp-content/plugins/better-wp-security/core/
class-itsec-files.php (418)
in ITSEC_Files::file_writer_init called at ? (?)
in call_user_func_array called at <path>/web/wp-includes/plugin.php (505)
in do_action called at <path>/web/wp-settings.php (237)
in require_once called at <path>/wp-config.php (180)
in require_once called at <path>/web/wp-load.php (34)
in require_once called at <path>/web/wp-blog-header.php (12)
in require called at <path>/web/index.php (17)

It seems to be stuff that calls the site options too early at a guess. It seems to happening when sites have not been hit for a while and get accessed. Like first person to hit in the morning or a Sunday evening....

#15 @wonderboymusic
4 years ago

  • Milestone changed from Future Release to 4.3
  • Owner set to wonderboymusic
  • Status changed from reopened to assigned
  • Type changed from enhancement to defect (bug)

#16 @wonderboymusic
4 years ago

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

In 32943:

In get_site_option() and get_option(), ensure that $notoptions is an array before writing to it. Prevents a flood of Cannot use a scalar value as an array, because $notoptions is otherwise set to the result of wp_cache_get(), which returns mixed.

Props hauvong.
Fixes #31147.

Note: See TracTickets for help on using tickets.