WordPress.org

Make WordPress Core

Opened 18 months ago

Closed 18 months ago

Last modified 18 months ago

#22223 closed defect (bug) (fixed)

Potential for post cache corruption since r21735

Reported by: ryan Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version:
Component: Cache API Keywords:
Focuses: Cc:

Description

Since r21735, the potential for corrupting the cache with bad post objects is much higher. For example, when looping over posts that are not all for the same blog, posts that do not have a complete set of properties, posts that have extra properties, and dummy posts that don't actually exist in the db. All of these scenarios are proving common enough to be a problem.

Attachments (6)

22223.diff (1.4 KB) - added by ryan 18 months ago.
Require properties to match
22223-ut.diff (1.1 KB) - added by ryan 18 months ago.
wp-post-props.diff (2.5 KB) - added by wonderboymusic 18 months ago.
22223.2.diff (473 bytes) - added by ryan 18 months ago.
Don't wp_cache_add()
22223.3.diff (2.8 KB) - added by ryan 18 months ago.
Remove wp_cache_add(). Add properties.
22223-ut.2.diff (966 bytes) - added by ryan 18 months ago.

Download all attachments as: .zip

Change History (26)

comment:1 follow-up: scribu18 months ago

Famous last words: we could just remove the wp_cache_add() call.

ryan18 months ago

Require properties to match

ryan18 months ago

comment:2 ryan18 months ago

The strictest way is to cache only if the object contains the core properties and only the core properties. If we do such a thing it'd be nice to skip the checks when calling get_post() in WP_Query::get_posts() since the objects there are safe.

comment:3 in reply to: ↑ 1 ryan18 months ago

Replying to scribu:

Famous last words: we could just remove the wp_cache_add() call.

I've been thinking the same. If the post came from a previous get_post(), from WP_Query, or from another core function it will already be cached. If it didn't come from a core function it is very likely to be something we don't want to cache.

comment:4 follow-up: wonderboymusic18 months ago

I feel like we're getting in "is_a" territory, is there a compelling reason to have to have public props with defaults like the diff I just attached?

(is_a meaning it implements an expected interface - if it can't be missing properties)

Last edited 18 months ago by wonderboymusic (previous) (diff)

comment:5 follow-up: ryan18 months ago

However, if both cache_results and split_the_query in WP_Query::get_posts() are false then the posts won't be added to the cache if get_post() doesn't wp_cache_add(). An environment using a persistent cache backend and a plugin such as advanced post cache that hasn't been updated to work with the split query would fit this scenario (cough, wordpress.com).

comment:6 in reply to: ↑ 4 ryan18 months ago

Replying to wonderboymusic:

I feel like we're getting in "is_a" territory, is there a compelling reason to have to have public props with defaults like the diff I just attached?

(is_a meaning it implements an expected interface - if it can't be missing properties)

Since we decided to not use __get() for core properties, we should consider making these proper properties regardless of this ticket. Adding default while we're at it would certainly solve part of the problem, that of missing properties. There are still the other two issues -- garbage properties and post objects not intended for the current blog.

Version 0, edited 18 months ago by ryan (next)

comment:7 ryan18 months ago

Something like $post->filter = ‘no-cache’ might be useful for those situations where someone knows they are fudging a post object and don't want it to ever end up in cache.

comment:8 in reply to: ↑ 5 ryan18 months ago

Replying to ryan:

However, if both cache_results and split_the_query in WP_Query::get_posts() are false then the posts won't be added to the cache if get_post() doesn't wp_cache_add(). An environment using a persistent cache backend and a plugin such as advanced post cache that hasn't been updated to work with the split query would fit this scenario (cough, wordpress.com).

This would be good, in part, because it would restore the 3.4 intent behind cache_results = false. get_post() in the array_map() would no longer be going behind its back and doing a bunch of wp_cache_add() calls anyway. But that would leave get_post() by post ID as the only way to get posts into cache. I suspect that passing objects to get_post() while rolling through The Loop is the main way posts are added to cache when cache_results = false, however.

Last edited 18 months ago by ryan (previous) (diff)

comment:9 scribu18 months ago

But that would leave get_post() by post ID as the only way to get posts into cache.

We need to make update_post_cache() work with a single post as well, then.

comment:10 follow-up: scribu18 months ago

Actually, I think setting the cache only when passing an ID to get_post() is the sane thing to do.

Beyond that, just let WP_Query handle the pre-caching, using update_post_caches().

comment:11 ryan18 months ago

Note that cache corruption was possible in 3.4 with get_post( $post ). The difference in 3.5 is that get_post() can corrupt the cache with the global post. It'd be nice to address both, but just fixing the global post would be sufficient.

comment:12 in reply to: ↑ 10 ; follow-up: ryan18 months ago

Replying to scribu:

Actually, I think setting the cache only when passing an ID to get_post() is the sane thing to do.

It does fix all problems and does so by removing a single line of code.

comment:13 in reply to: ↑ 12 nacin18 months ago

Replying to ryan:

Replying to scribu:

Actually, I think setting the cache only when passing an ID to get_post() is the sane thing to do.

It does fix all problems and does so by removing a single line of code.

I like this.

ryan18 months ago

Don't wp_cache_add()

comment:14 follow-up: ryan18 months ago

We might still want to use proper properties with defaults. We're seeing some homebrew objects that don't even have post_type set, resulting in warnings down the line and extra queries because the new magic get tries to fetch post_type from meta.

comment:15 in reply to: ↑ 14 nacin18 months ago

Replying to ryan:

We might still want to use proper properties with defaults. We're seeing some homebrew objects that don't even have post_type set, resulting in warnings down the line and extra queries because the new magic get tries to fetch post_type from meta.

Would __get() fire in that case? The property is declared, so __get() would only get hit if someone takes a WP_Post object and then unsets a property.

comment:16 nacin18 months ago

Oops, I thought that the properties were declared, but that's actually the inline blocks. Declaring the properties — even without default values — is enough to avoid it ever hitting __get().

comment:17 scribu18 months ago

+1 for declaring the properties, but maybe without all the verbose doc comments.

comment:18 wonderboymusic18 months ago

We don't need to go J-trip on it, but everything in the codebase should have Doc blocks on it for at least a fighting chance of generating usable docs

ryan18 months ago

Remove wp_cache_add(). Add properties.

ryan18 months ago

comment:19 ryan18 months ago

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

In [22264]:

Don't wp_cache_add() stdClass objects in get_post() to avoid polluting the cache with incomplete or otherwise compromised objects.

Declare the core properties of WP_Pist as proper public properties and provide them with defaults.

Props wonderboymusic
fixes #22223

Note: See TracTickets for help on using tickets.