WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 6 months ago

#34913 new defect (bug)

Unscheduling cron jobs fails when original arguments were not an array.

Reported by: jrf Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.0
Component: Cron API Keywords: has-patch has-unit-tests 2nd-opinion
Focuses: Cc:

Description

The Cron API does not check whether the cron job arguments passed are an array when scheduling a cron job. This inadvertently allows for scheduling cron jobs with string, integer or other arguments.

However when unscheduling the cron job using wp_clear_scheduled_hook(), the arguments are always cast to an array which leads to cron jobs which *can* be scheduled, but can't be *un*scheduled using wp_clear_scheduled_hook().

The wp_clear_scheduled_hook() does throw a deprecated notices when non-array arguments are passed in, but this will most of the time go unnoticed as this function is most often used in a plugin deactivation routine.

The patch which I'm submitting makes sure that cron job arguments are always cast to an array.

The patch is backward compatible in that it:

  • will not break the schedule_event filter for plugins (which are doing it wrong) which expect their original non-array argument to test against.
  • will schedule all newly schedule events with array arguments independently of how the arguments were passed.
  • will upgrade the cron array to ensure that all arguments are arrays.

The patch includes unit tests proving the existence of the bug and the fixing of it by this patch.

As far as I can see, this bug was introduced by the changes in https://core.trac.wordpress.org/changeset/12462 and has been in WP since 3.0.

The patching of this bug also brought to my attention *another* (ancient) bug where in the cron option upgrade routine _upgrade_cron_array() the array structure wasn't respected properly leading to Undefined index: args notices and the inadvertent removal of cron events which were scheduled on the same hook for the same timestamp with different arguments.

That bug has also been fixed in this patch.

Attachments (6)

0001-Fix-issue-34913.patch (9.2 KB) - added by jrf 2 years ago.
0001-Fix-issue-34913.2.patch (9.2 KB) - added by jrf 2 years ago.
0002-a-Fix-issue-34913.patch (10.0 KB) - added by jrf 22 months ago.
Updated for bug scrub feedback
0002-b-Fix-issue-34913-with-doing-it-wrong.patch (12.6 KB) - added by jrf 22 months ago.
Same as 2a + _doing_it_wrong()
wp_next_scheduled.3.patch (2.9 KB) - added by jrf 22 months ago.
Refreshed against current master
0002-b-Fix-issue-34913-with-doing-it-wrong-refreshed.patch (12.3 KB) - added by jrf 22 months ago.
Refreshed against current master

Download all attachments as: .zip

Change History (17)

#1 @jrf
2 years ago

  • Keywords has-patch has-unit-tests added

#2 @jrf
2 years ago

Refreshed the patch to include the ticket number in the docblock of the unit tests.

#3 @mikeschroder
2 years ago

  • Version changed from trunk to 3.0

#4 @jrf
2 years ago

Oh and FYI - this patch also takes care of the following PHP warnings:

PHP Warning:  array_slice() expects parameter 1 to be array, string given in /wp-includes/plugin.php on line 601

PHP Stack trace:
PHP   1. {main}() /wp-cron.php:0
PHP   2. do_action_ref_array($tag = 'testing', $args = 'STRING SINGLE Testing string arg') /wp-cron.php:117
PHP   3. array_slice('STRING SINGLE Testing string arg', 0, 1) /wp-includes/plugin.php:601


PHP Warning:  call_user_func_array() expects parameter 2 to be array, null given in /wp-includes/plugin.php on line 601

PHP Stack trace:
PHP   1. {main}() /wp-cron.php:0
PHP   2. do_action_ref_array($tag = 'testing', $args = 'STRING SINGLE Testing string arg') /wp-cron.php:117
PHP   3. call_user_func_array:{/wp-includes/plugin.php:601}('testing', NULL) /wp-includes/plugin.php:601
Last edited 2 years ago by jrf (previous) (diff)

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


2 years ago

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


22 months ago

@jrf
22 months ago

Updated for bug scrub feedback

@jrf
22 months ago

Same as 2a + _doing_it_wrong()

#7 @jrf
22 months ago

Updated the patch based on feedback from the bug scrub Slack chat. /cc @chriscct7 @stevenkword

I think the patch should also throw a _doing_it_wrong() specifically where the plugin authors are doing it wrong

There is already a _deprecated_argument() in place in the wp_clear_scheduled_hook() function which was added when the function signature was changed.

I could add _doing_it_wrong() for all instances where the cron arguments are not passed in as an array. That would effectively mean adding it in six (!) places in the cron file.
See the alternative patch (2-b).

Also note: _doing_it_wrong() expects the WP version to be the version in which the message was added (4.5 (?)), however, the actual change in the function signature occurred in WP 2.1.0 as far as I can see, so IMHO it would make more sense to have 2.1.0 as the version number in the _doing_it_wrong() call.

also it'd be nice to get the related ticket ( *another* (ancient) bug), so that ticket can also be updated.

AFAIK this was never reported before and there is no trac ticket for this bug. I discovered it while working on this issue.

As the last time the version nr of the cron array was changed - and therefore the last time the _upgrade_cron_array() function was called -, was in 2006, I doubt you will find any reported issues.

The bug, however, becomes relevant for this patch as the cron array version nr will be upped and therefore _upgrade_cron_array() will now once again be called (once for each install).

Aside from the error notice, the net effect of this bug is cron events disappearing or no longer functioning correctly as the cron array would be mangled after being run through the upgrade function.

In more detail:
The cron array is a nested array with:
(array) timestamps -> (array) hooks -> (array) events -> (array) arguments, where events are saved with as a key the md5-ed serialization of the event arguments and as a value the actual event arguments.

The _upgrade_cron_array() function however treated the events array as if it were the arguments array. So if there were two events on a hook, there would only be one event after the upgrade routine and the arguments of that one event would be the previous two events.

and the new _cron_cast_to_array_helper needs a docbloc

Done - see updated patch.

#8 @swissspidy
22 months ago

  • Keywords 2nd-opinion added

The Cron API does not check whether the cron job arguments passed are an array when scheduling a cron job. This inadvertently allows for scheduling cron jobs with string, integer or other arguments.

How many plugins really do this? Is it worth papering over their faulty code and properly enabling non-array arguments? Couldn't we just call _doing_it_wrong() and return false?

See also #35491, where I suggested passing false to wp_next_scheduled() in order to get the next scheduled event for a hook regardless of the arguments. This patch here would prevent that from happening.

@jrf
22 months ago

Refreshed against current master

@jrf
22 months ago

Refreshed against current master

#10 @jrf
22 months ago

Please ignore the wp_next_schedules.3.patch file - my bad, uploaded the wrong patch and there is no way for me to delete it....

#11 @jrf
6 months ago

Just wondering if this patch can get some love ? I'd be happy to update it if someone would give the go-ahead for 4.9.

Note: See TracTickets for help on using tickets.