Make WordPress Core

Opened 18 months ago

Closed 16 months ago

Last modified 12 months ago

#57593 closed defect (bug) (fixed)

Warn about calling `_get_non_cached_ids()` with invalid ids

Reported by: tillkruess's profile tillkruess Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: major Version: 5.5
Component: Cache API Keywords: has-patch has-unit-tests commit early has-dev-note
Focuses: performance Cc:

Description

Currently the _get_non_cached_ids() function doesn't validate its $object_ids argument contains only int[]. This can lead to call like:

wp_cache_get_multiple( [ null, false, true, 1, 2, 3 ], 'posts' );

or even worse:

wp_cache_get_multiple( [ 1, 2, array() ], 'posts' );

Which is impossible to translate to it's expected return value:

[
    1 => 1,
    2 => 2,
    array() => 0
];

Similar to #56198 we should check the given $object_ids and at least call _doing_it_wrong().

Change History (25)

#1 @tillkruess
18 months ago

Maybe we can turn WP_Object_Cache::is_valid_key() into a global helper function and use it in _get_non_cached_ids()?

#2 @spacedmonkey
18 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from trunk to 5.5

How about a simplier solution like this?

function _get_non_cached_ids( $object_ids, $cache_key ) {
        $non_cached_ids = array();
        $object_ids = array_filter( $object_ids, 'is_scalar' );

[48055] for extra context.

#3 follow-up: @tillkruess
18 months ago

@spacedmonkey: That would allow bool keys, which often groups false into the same key unexpectedly.

#4 in reply to: ↑ 3 @spacedmonkey
18 months ago

Replying to tillkruess:

@spacedmonkey: That would allow bool keys, which often groups false into the same key unexpectedly.

$object_ids = array_filter( $object_ids, function( $object_id ) {
   return is_scalar($object_id) && !is_bool( $object_id );
});

How about this?

#5 @peterwilsoncc
18 months ago

Does $object_ids = array_filter( $object_ids, 'is_numeric' /* or 'is_int' */ ); work?

#6 @tillkruess
18 months ago

Yeah, I think is_numeric() would be strict-ish enough.

Most edge cases like 42.0 or "1337e0" are so rare, we only really need to protect against false and null and other gross type mismatches.

Should we just do an array_filter() or also throw a little user notice?

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

This ticket was mentioned in PR #4076 on WordPress/wordpress-develop by @tillkruess.


17 months ago
#7

  • Keywords has-patch added; needs-patch removed

Remove entries from $object_ids array that are not integers or a string contain an integer.

Trac ticket: https://core.trac.wordpress.org/ticket/57593

#8 follow-up: @tillkruess
17 months ago

I've opened a PR with a simple fix.

Since _get_non_cached_ids() is no longer marked as private, should we inform the user that a non-integer was given using a _doing_it_wrong() call, like we did here? Otherwise this may lead to silent misbehavior.

#9 in reply to: ↑ 8 @peterwilsoncc
17 months ago

Replying to tillkruess:

Since _get_non_cached_ids() is no longer marked as private, should we inform the user that a non-integer was given using a _doing_it_wrong() call, like we did here? Otherwise this may lead to silent misbehavior.

Yes, let's go with consistency.

@tillkruess commented on PR #4076:


17 months ago
#11

I don't understand the yoda failure.

@peterwilsoncc commented on PR #4076:


17 months ago
#12

Oh, some additional tests would be helpful here too.

Something to ensure that the expected failures throw the notice and that everything that should be valid does not. IS that something you have bandwidth to help out with?

@peterwilsoncc commented on PR #4076:


17 months ago
#13

🤦 Looks like an off-by-one error with the yoda note. */ is definitely not triggering it. Let's suppress the error with a note it's a false positive.

@tillkruess commented on PR #4076:


17 months ago
#14

🤦 Looks like an off-by-one error with the yoda note. */ is definitely not triggering it. Let's suppress the error with a note it's a false positive.

How? 😬

@peterwilsoncc commented on PR #4076:


17 months ago
#15

Hey Till,

After chatting in Slack, I took the liberty of pushing a few changes to the tests:

  • moved the tests to a dedicated file for the function -- apparently it hasn't been tested directly in the past
  • expanded the data provider for invalid object IDs to include a few extra types.
  • switched the group name to fake-group to avoid collisions/bad luck causing the tests to fail
  • added a test for valid ids to make sure they were returned as expected (ie, integers)

I've also confirmed on trunk that wp eval "var_dump( _get_non_cached_ids( array( 500, '501' ), 'invalidgroup' ) );" returns integers and this doesn't change any return types. It does not, so that's nice :)

@tillkruess commented on PR #4076:


17 months ago
#16

@SergeyBiryukov @spacedmonkey @felixarntz Read for review ✌️

#17 @spacedmonkey
17 months ago

  • Milestone changed from Future Release to 6.3

@spacedmonkey commented on PR #4076:


17 months ago
#18

@tillkruss That is not what I had in mind. I had this in mind. https://github.com/WordPress/wordpress-develop/commit/b9d0ad5d504f77e0f417ec0aa458eee1ba602a5c. This means no static callbacks and we can unit tests directly against, _sanaitize_cache_id.

@tillkruess commented on PR #4076:


17 months ago
#19

@tillkruss That is not what I had in mind. I had this in mind. b9d0ad5. This means no static callbacks and we can unit tests directly against, _sanaitize_cache_id.

I'm so confused, where did this commit go?!

#20 @spacedmonkey
17 months ago

  • Keywords has-unit-tests has-dev-note commit early added
  • Owner set to spacedmonkey
  • Status changed from new to assigned

Marking as ready to commit. Assigning to myself as committer, will commit as part of 6.3 release.

#21 @spacedmonkey
16 months ago

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

In 55543:

Cache API: Add a warning when calling _get_non_cached_ids with invalid ids.

Sanitize the array of ids passed to the _get_non_cached_ids function and add a _doing_it_wrong call, if an invalid type is passed.

Props tillkruess, spacedmonkey, peterwilsoncc, flixos90, SergeyBiryukov.
Fixes #57593.

#22 @SergeyBiryukov
16 months ago

In 55549:

Docs: Fix typo in _validate_cache_id() description.

Includes:

  • Capitalizing "ID" in a consistent way.
  • Expanding the comment on not using filter_var().
  • Adding a @covers tag for the function in unit tests.
  • Minor tweak to the _doing_it_wrong() message.

Follow-up to [53818], [55543].

See #57593.

#24 @spacedmonkey
13 months ago

  • Focuses performance added
  • Keywords needs-dev-note added; has-dev-note removed

#25 @flixos90
12 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.