Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 3 years ago

#28102 closed defect (bug) (fixed)

is_string check in validate_file

Reported by: iwanluijksquestmedia's profile IwanLuijksQuestMedia Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.0 Priority: normal
Severity: normal Version: 2.2
Component: Plugins Keywords: has-patch commit
Focuses: administration Cc:

Description

When creating a plugin there is the possibility to get an object in validate_file's first parameter, since there is no check on the parameters this function accepts, it triggers the following warning. Worse: the actual plugin activation process keeps adding the plugin to the active_plugins list in the *_options table in result.

Warning: strpos() expects parameter 1 to be string, object given in /<path>/wp-includes/functions.php on line 3345
Call Stack:
0.0009 937456 1. {main}() /<path>/wp-admin/plugins.php:0
0.0013 1053776 2. require_once('/<path>/wp-admin/admin.php') /<path>/wp-admin/plugins.php:10
0.0014 1079792 3. require_once('/<path>/wp-load.php') /<path>/wp-admin/admin.php:30
0.0015 1100288 4. require_once('/<path>/wp-config.php') /<path>/wp-load.php:29
0.0019 1240920 5. require_once('/<path>/wp-settings.php') /<path>/wp-config.php:92
0.0969 26021992 6. wp_get_active_and_valid_plugins() /<path>/wp-settings.php:212
0.0971 26023184 7. validate_file() /<path>/wp-includes/load.php:506
0.0971 26023392 8. strpos() /<path>/wp-includes/functions.php:3345

@@ -3342,6 +3342,9 @@
  * @return int 0 means nothing is wrong, greater than 0 means something was wrong.
  */
 function validate_file( $file, $allowed_files = '' ) {
+       if (!is_string($file) || ($allowed_files !== '' && !is_array($allowed_files)))
+               return 4;
+
        if ( false !== strpos( $file, '..' ) )
                return 1;

Attachments (2)

28102.patch (704 bytes) - added by SergeyBiryukov 10 years ago.
28102.2.patch (711 bytes) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (11)

#1 follow-up: @IwanLuijksQuestMedia
10 years ago

This happens when in the plugin itself you use the variable $plugin, as in:

<?php
$plugin = \SomeQM\Plugin::instantiate();
$plugin->expose_hooks();

While the following doesn't trigger the errors:

<?php
$plugin2 = \SomeQM\Plugin::instantiate();
$plugin2->expose_hooks();

This is really bad side-effect behaviour, and should be fixed, or at least fully documented somewhere.

#2 @juliobox
10 years ago

Hello

So you're telling us that when you badly use a function, there is some PHP Warnings? Don't you say! :)
This is not even a WordPress bug, because you can not reproduce it without a plugin/script badly written, am i wrong?

The documentation is already there :

 * @param string $file File path.
 * @param array $allowed_files List of allowed files.
 * @return int 0 means nothing is wrong, greater than 0 means something was wrong.
 */
function validate_file( $file, $allowed_files = '' ) {

1st param is "string" as far as i can read.
So, i don't think that each function have to check each type of the given parameters, just use it correctly, unless it works in a clean WP install.

Thank you

#3 in reply to: ↑ 1 @SergeyBiryukov
10 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.0
  • Version changed from 3.9 to 2.2

Replying to IwanLuijksQuestMedia:

This happens when in the plugin itself you use the variable $plugin

Confirmed. $plugin = array() in a plugin file causes a bunch of notices on activation:

Notice: Array to string conversion in S:\home\wordpress\trunk\wp-includes\functions.php on line 3346
Notice: Array to string conversion in S:\home\wordpress\trunk\wp-includes\functions.php on line 3349
Notice: Array to string conversion in S:\home\wordpress\trunk\wp-includes\functions.php on line 3355
Notice: Array to string conversion in S:\home\wordpress\trunk\wp-includes\load.php on line 507

Notice: Array to string conversion in S:\home\wordpress\trunk\wp-includes\functions.php on line 3346
Notice: Array to string conversion in S:\home\wordpress\trunk\wp-includes\functions.php on line 3349
Notice: Array to string conversion in S:\home\wordpress\trunk\wp-includes\functions.php on line 3355
Warning: Illegal offset type in S:\home\wordpress\trunk\wp-admin\includes\plugin.php on line 874

It also leads to a corrupted active_plugins option, which can only be fixed by a manual reset:

Array(
    [0] => akismet/akismet.php
    [1] => debug-bar/debug-bar.php
    [2] => hello.php
    [3] => Array()
)

This is caused by accidentally overriding the local $plugin variable in activate_plugin():
tags/3.9/src/wp-admin/includes/plugin.php#L536.

Appears to be introduced in [4811], which changed the calling sequence. Also related: #6308.

28102.patch fixes this by using a more unique name, $_wp_plugin_file, to restore the previous value.

We could probably just replace all instances of $plugin in activate_plugin() with $_wp_plugin_file, but $plugin seems more readable.

Last edited 10 years ago by SergeyBiryukov (previous) (diff)

#4 @SergeyBiryukov
10 years ago

  • Keywords commit added

28102.2.patch adds a better comment.

#5 @SergeyBiryukov
10 years ago

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

In 28644:

Avoid stomping of the $plugin variable in activate_plugin().

fixes #28102.

#6 @juliobox
10 years ago

Same problem with $_wp_plugin_file = array(); now. So?

#7 @SergeyBiryukov
10 years ago

The idea is to use a variable with much smaller chances of accidental stomping than $plugin.

#8 @nacin
9 years ago

Thanks for the clear code comment in [28644].

Note: See TracTickets for help on using tickets.