WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#24210 closed defect (bug) (fixed)

Issues found using a static analysis tool

Reported by: rlerdorf Owned by: nacin
Milestone: 3.6 Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: Cc:

Description (last modified by nacin)

These all look like valid, but minor, issues:

--------------------------------
File       : wp-includes/class-json.php:495
Reason     : UnknownFunction
Snippet    : class_name($var)
Line       : : new Services_JSON_Error(class_name($var).

--------------------------------
File       : wp-includes/SimplePie/Locator.php:94
Reason     : RequiredAfterOptionalParam
Snippet    : $type = SIMPLEPIE_LOCATOR_ALL
Line       : public function find($type = SIMPLEPIE_LOCATOR_ALL, &$working)

--------------------------------
File       : wp-includes/ID3/module.tag.id3v2.php:433
Reason     : TooFewArgument
Snippet    : substr($footer[5])
Line       : $id3_flags = ord(substr($footer{5}));

--------------------------------
File       : wp-includes/ID3/module.tag.id3v2.php:1586
Reason     : StatementHasNoEffect
Snippet    : $frame_ownerid == '';
Line       : $frame_ownerid == '';

--------------------------------
File       : wp-includes/ID3/module.audio.mp3.php:37
Reason     : TooManyArgument
Snippet    : $this->getOnlyMPEGaudioInfoBruteForce($this->getid3->fp, $info)
Line       : $this->getOnlyMPEGaudioInfoBruteForce($this->getid3->fp, $info);

--------------------------------
File       : wp-includes/SimplePie/Misc.php:127
Reason     : TooManyArgument
Snippet    : SimplePie_Misc::entities_decode(end($attribs[$j]), 'UTF-8')
Line       : $return[$i]['attribs'][strtolower($attribs[$j][1])]['data'] = SimplePie_Misc::entities_decode(end($attribs[$j]), 
'UTF-8');

Resolved:

--------------------------------
File       : wp-includes/class-wp-walker.php:118
Reason     : RequiredAfterOptionalParam
Snippet    : $depth = 0
Line       : function display_element( $element, &$children_elements, $max_depth, $depth=0, $args, &$output ) {

--------------------------------
File       : wp-includes/comment-template.php:1298
Reason     : RequiredAfterOptionalParam
Snippet    : $depth = 0
Line       : function display_element( $element, &$children_elements, $max_depth, $depth=0, $args, &$output ) {

--------------------------------
File       : wp-includes/deprecated.php:802
Reason     : RequiredAfterOptionalParam
Snippet    : $echo = false
Line       : function get_author_link($echo = false, $author_id, $author_nicename = '') {

--------------------------------
File       : wp-includes/deprecated.php:1709
Reason     : TooManyArgument
Snippet    : get_the_content($more_link_text, $stripteaser, $more_file)
Line       : $content = get_the_content($more_link_text, $stripteaser, $more_file);

--------------------------------
File       : wp-signup.php:493
Reason     : RequiredAfterOptionalParam
Snippet    : $user_name = ''
Line       : function confirm_blog_signup($domain, $path, $blog_title, $user_name = '', $user_email = '', $meta) {

--------------------------------
File       : wp-signup.php:493
Reason     : RequiredAfterOptionalParam
Snippet    : $user_email = ''
Line       : function confirm_blog_signup($domain, $path, $blog_title, $user_name = '', $user_email = '', $meta) {

--------------------------------
File       : wp-includes/widgets.php:76
Reason     : RequiredAfterOptionalParam
Snippet    : $id_base = false
Line       : function WP_Widget( $id_base = false, $name, $widget_options = array(), $control_options = array() ) {

--------------------------------
File       : wp-includes/widgets.php:93
Reason     : RequiredAfterOptionalParam
Snippet    : $id_base = false
Line       : function __construct( $id_base = false, $name, $widget_options = array(), $control_options = array() ) {

--------------------------------
File       : wp-includes/post.php:4789
Reason     : RequiredAfterOptionalParam
Snippet    : $deprecated = ''
Line       : function _future_post_hook( $deprecated = '', $post ) {

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:173
Reason     : RequiredAfterOptionalParam
Snippet    : $start = 0
Line       : function _rows( $taxonomy, $terms, &$children, $start = 0, $per_page = 20, &$count, $parent = 0, $level = 0 ) {

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:173
Reason     : RequiredAfterOptionalParam
Snippet    : $per_page = 20
Line       : function _rows( $taxonomy, $terms, &$children, $start = 0, $per_page = 20, &$count, $parent = 0, $level = 0 ) {

--------------------------------
File       : wp-includes/rewrite.php:92
Reason     : TooManyArgument
Snippet    : remove_action($hook, $hook, 10, 1)
Line       : remove_action($hook, $hook, 10, 1);

--------------------------------
File       : wp-admin/includes/user.php:350
Reason     : TooManyArgument
Snippet    : delete_user_setting('default_password_nag', $user_ID)
Line       : delete_user_setting('default_password_nag', $user_ID);

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:159
Reason     : TooManyArgument
Snippet    : $this->single_row($term, 0, $taxonomy)
Line       : $out .= $this->single_row( $term, 0, $taxonomy );

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:202
Reason     : TooManyArgument
Snippet    : $this->single_row($my_parent, $level - $num_parents, $taxonomy)
Line       : $output .=  "\t" . $this->single_row( $my_parent, $level - $num_parents, $taxonomy );

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:208
Reason     : TooManyArgument
Snippet    : $this->single_row($term, $level, $taxonomy)
Line       : $output .= "\t" . $this->single_row( $term, $level, $taxonomy );

--------------------------------
File       : wp-includes/media.php:2453
Reason     : UnknownFunction
Snippet    : sprint($link_fmt, $image)
Line       : $image = sprint( $link_fmt, $image );

--------------------------------
File       : wp-includes/media.php:1040
Reason     : TooManyArgument
Snippet    : wp_mediaelement_fallback($fileurl, $width, $height)
Line       : $html .= wp_mediaelement_fallback( $fileurl, $width, $height );

--------------------------------
File       : wp-includes/Text/Diff/Engine/xdiff.php:30
Reason     : UnknownFunction
Snippet    : xdiff_string_diff($from_string, $to_string, count($to_lines))
Line       : $diff = xdiff_string_diff($from_string, $to_string, count($to_lines));

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:159
Reason     : UseVoidReturn
Snippet    : $this->single_row($term, 0, $taxonomy)
Line       : $out .= $this->single_row( $term, 0, $taxonomy );

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:202
Reason     : UseVoidReturn
Snippet    : $this->single_row($my_parent, $level - $num_parents, $taxonomy)
Line       : $output .=  "\t" . $this->single_row( $my_parent, $level - $num_parents, $taxonomy );

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:208
Reason     : UseVoidReturn
Snippet    : $this->single_row($term, $level, $taxonomy)
Line       : $output .= "\t" . $this->single_row( $term, $level, $taxonomy );

--------------------------------
File       : wp-admin/includes/class-wp-terms-list-table.php:228
Reason     : UseVoidReturn
Snippet    : $this->single_row_columns($tag)
Line       : echo $this->single_row_columns( $tag );

--------------------------------
File       : wp-includes/wp-db.php:648
Reason     : TooManyArgument
Snippet    : $this->has_cap('collation', $dbh)
Line       : if ( $this->has_cap( 'collation', $dbh ) && !empty( $charset ) ) {

--------------------------------
File       : wp-includes/wp-db.php:649
Reason     : TooManyArgument
Snippet    : $this->has_cap('set_charset', $dbh)
Line       : if ( function_exists( 'mysql_set_charset' ) && $this->has_cap( 'set_charset', $dbh ) ) {

--------------------------------
File       : wp-admin/network/site-settings.php:63
Reason     : TooManyArgument
Snippet    : update_option($key, $val, false)
Line       : update_option( $key, $val, false ); // no need to refresh blog details yet

--------------------------------
File       : wp-admin/network/site-settings.php:126
Reason     : TooManyArgument
Snippet    : esc_html(maybe_unserialize($option->option_value), 'single')
Line       : $option->option_value = esc_html( maybe_unserialize( $option->option_value ), 'single' );

--------------------------------
File       : wp-admin/includes/class-wp-comments-list-table.php:318
Reason     : UseVoidReturn
Snippet    : $this->single_row_columns($comment)
Line       : echo $this->single_row_columns( $comment );

--------------------------------
File       : wp-admin/includes/class-wp-list-table.php:829
Reason     : UseVoidReturn
Snippet    : $this->single_row_columns($item)
Line       : echo $this->single_row_columns( $item );

--------------------------------
File       : wp-admin/includes/class-wp-upgrader.php:1132
Reason     : UseVoidReturn
Snippet    : screen_icon()
Line       : echo screen_icon();

--------------------------------
File       : wp-admin/includes/class-wp-posts-list-table.php:386
Reason     : UseVoidReturn
Snippet    : $this->single_row($page, $level)
Line       : echo "\t" . $this->single_row( $page, $level );

--------------------------------
File       : wp-admin/includes/class-wp-posts-list-table.php:401
Reason     : UseVoidReturn
Snippet    : $this->single_row($op, 0)
Line       : echo "\t" . $this->single_row( $op, 0 );

--------------------------------
File       : wp-admin/includes/class-wp-posts-list-table.php:447
Reason     : UseVoidReturn
Snippet    : $this->single_row($my_parent, $level - $num_parents)
Line       : echo "\t" . $this->single_row( $my_parent, $level - $num_parents );

--------------------------------
File       : wp-admin/includes/class-wp-posts-list-table.php:453
Reason     : UseVoidReturn
Snippet    : $this->single_row($page, $level)
Line       : echo "\t" . $this->single_row( $page, $level );

Attachments (3)

24210.patch (5.6 KB) - added by SergeyBiryukov 2 years ago.
24210.2.patch (6.8 KB) - added by SergeyBiryukov 2 years ago.
24210.wpdb.patch (1.9 KB) - added by SergeyBiryukov 2 years ago.

Download all attachments as: .zip

Change History (43)

comment:1 follow-up: @rlerdorf2 years ago

Actually, ignore the xdiff_string_diff() and class_name() ones. Those slipped through my filters. But the others are valid.

comment:2 @SergeyBiryukov2 years ago

  • Description modified (diff)

comment:3 @SergeyBiryukov2 years ago

In 24118:

Fix typo in get_the_post_format_image(). props rlerdorf. see #23964. see #24210.

comment:4 @SergeyBiryukov2 years ago

  • Milestone changed from Awaiting Review to 3.6

24210.patch fixes the usage of WP_List_Table::single_row() in echo context.

@SergeyBiryukov2 years ago

@SergeyBiryukov2 years ago

comment:5 @SergeyBiryukov2 years ago

24210.2.patch handles all the UseVoidReturn instances.

WP_Users_List_Table::single_row() didn't require the change, as it actually returns the output, apparently for wp_ajax_add_user(), although that function doesn't appear to be used anywhere.

WP_Terms_List_Table::_rows(), on the other hand, does not need to return the output and currently mixes string concatenation with direct echoing. The patch resolves that.

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

comment:6 follow-up: @SergeyBiryukov2 years ago

File       : wp-admin/includes/user.php:350
Reason     : TooManyArgument
Snippet    : delete_user_setting('default_password_nag', $user_ID)
Line       : delete_user_setting('default_password_nag', $user_ID);

Introduced in [14608]. Unlike get_user_option(), delete_user_setting() does not accept a second parameter, so it deletes the setting for current user.

comment:7 @SergeyBiryukov2 years ago

File       : wp-includes/wp-db.php:648
Reason     : TooManyArgument
Snippet    : $this->has_cap('collation', $dbh)
Line       : if ( $this->has_cap( 'collation', $dbh ) && !empty( $charset ) ) {

Introduced in [15537]. wpdb::has_cap() only accepts one parameter.

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

comment:8 in reply to: ↑ 6 @ocean902 years ago

Replying to SergeyBiryukov:

Introduced in [14608]. Unlike get_user_option(), delete_user_setting() does not accept a second parameter, so it deletes the setting for current user.

All *_user_setting() functions doesn't accept an $user_id as a parameter since they referring to the current user cookie.

@SergeyBiryukov2 years ago

comment:9 @SergeyBiryukov2 years ago

24210.wpdb.patch handles the changes in wp-db.php.

comment:10 @SergeyBiryukov2 years ago

getID3() issues should be reported upstream:
http://www.getid3.org/phpBB3/viewforum.php?f=4

comment:11 @SergeyBiryukov2 years ago

File       : wp-signup.php:493
Reason     : RequiredAfterOptionalParam
Snippet    : $user_name = ''
Line       : function confirm_blog_signup($domain, $path, $blog_title, $user_name = '', $user_email = '', $meta) {

confirm_blog_signup() accepts $meta parameter, but doesn't use it anywhere.

Introduced as show_create_confirm_form() in mu:543, renamed to confirm_blog_signup() in mu:557. $user_name and $user_email became optional in mu:1321.

Merged with WP in [12603].

Version 1, edited 2 years ago by SergeyBiryukov (previous) (next) (diff)

comment:12 @SergeyBiryukov2 years ago

File       : wp-admin/network/site-settings.php:126
Reason     : TooManyArgument
Snippet    : esc_html(maybe_unserialize($option->option_value), 'single')
Line       : $option->option_value = esc_html( maybe_unserialize( $option->option_value > ), 'single' );

Introduced in [13106]. wp_specialchars(..., 'single') was replaced with esc_html(), which only accepts one parameter.

comment:13 @SergeyBiryukov2 years ago

File       : wp-admin/network/site-settings.php:63
Reason     : TooManyArgument
Snippet    : update_option($key, $val, false)
Line       : update_option( $key, $val, false ); // no need to refresh blog details yet

Introduced in mu:1811. update_blog_option() was replaced with update_option(), which only accepts two parameters. The fourth parameter of update_blog_option() was deprecated in [16673].

comment:14 @SergeyBiryukov2 years ago

In 24120:

Remove extraneous function parameters in the network admin. props rlerdorf. see #24210.

comment:15 follow-up: @SergeyBiryukov2 years ago

In 24121:

Remove extraneous function parameters in the wpdb class. props rlerdorf. see #24210.

comment:16 @SergeyBiryukov2 years ago

In 24122:

Remove extraneous function parameters in wp_video_shortcode(). props rlerdorf. see #24210.

comment:17 @SergeyBiryukov2 years ago

In 24123:

Remove redundant echo calls from list tables. Don't mix string concatenation with direct output. see #24210.

comment:18 @SergeyBiryukov2 years ago

  • Description modified (diff)

comment:19 in reply to: ↑ 1 @SergeyBiryukov2 years ago

Replying to rlerdorf:

Actually, ignore the xdiff_string_diff() and class_name() ones. Those slipped through my filters. But the others are valid.

The class_name() one seems valid too.

Introduced in [23376]. Looks like get_class() should be used instead.

comment:20 @rlerdorf2 years ago

I guess so, yes. I'll do another run tomorrow and filter a bit less. I may have filtered out other valid issues.

comment:21 in reply to: ↑ 15 ; follow-up: @dd322 years ago

Replying to SergeyBiryukov:

In 24121:

Remove extraneous function parameters in the wpdb class. props rlerdorf. see #24210.

FWIW, This is a hold over from HyperDB I believe, as the has_cap has to be called on the individual table/database connection being tested: http://plugins.trac.wordpress.org/browser/hyperdb/trunk/db.php#L858

comment:22 @nacin2 years ago

  • Owner set to nacin
  • Status changed from new to accepted

comment:23 in reply to: ↑ 21 @SergeyBiryukov2 years ago

Replying to dd32:

FWIW, This is a hold over from HyperDB I believe, as the has_cap has to be called on the individual table/database connection being tested

Thanks for the clarification, makes sense.

HyperDB passes the handle to db_version(). wpdb, however, only supports a single connection, so it didn't accept the extra parameter in has_cap().

comment:24 @nacin2 years ago

In 24125:

delete_user_setting() and remove_action() were getting called with too many args. props rlerdorf. see #24210.

comment:25 @nacin2 years ago

In 24126:

Required arguments can't follow optional arguments.

Make required arguments optional in confirm_blog_signup().

Mark arguments as required in _future_post_hook(), the walker method display_element(), get_author_link() (deprecated), and the WP_Widget constructor.

props rlerdorf.
see #24210.

comment:26 @nacin2 years ago

In 24127:

Terms list table:

  • Don't call single_row() with an undeclared and unused $taxonomy argument.
  • Don't define optional parameters before required parameters in the _rows() method. Make them required.
  • Move empty( $terms ) check above other operations. This function was improperly returning an else case until [24123].

props rlerdorf.
see #24210.

comment:27 @nacin2 years ago

In 24128:

Remove an extra argument passed to get_the_content() in the deprecated the_content_rss().

props rlerdorf.
see #24210.

comment:28 @nacin2 years ago

  • Description modified (diff)

Thanks, Rasmus, for testing your tool on WordPress trunk.

The only ones left by my count are from ID3, Services_JSON, and SimplePie.

SimplePie upstream: https://github.com/simplepie/simplepie/issues/293 (open) and https://github.com/simplepie/simplepie/pull/291 (merged)

Services_JSON upstream: http://pear.php.net/bugs/bug.php?id=19920

The lack of recent activity upstream for ID3 (not to mention the use of a phpbb forum as a bug tracker) scares the hell out of me: http://www.getid3.org/phpBB3/viewforum.php?f=4

comment:29 follow-up: @rlerdorf2 years ago

Is there a branch that has these fixes? I ran the static analyzer again against master and it still sees a lot of issues. The current list is here:

https://gist.github.com/rlerdorf/5530518

Looks like there are some new ones on the list too. Probably because I have been massaging the filters.

Last edited 2 years ago by rlerdorf (previous) (diff)

comment:30 in reply to: ↑ 29 @nacin2 years ago

Replying to rlerdorf:

Is there a branch that has these fixes? I ran the static analyzer again against master and it still sees a lot of issues. The current list is here:

https://gist.github.com/rlerdorf/5530518

Looks like there are some new ones on the list too. Probably because I have been massaging the filters.

All of those are new, except a few pending upstream ones. Many of them have the reason UseUndeclaredVariable, which was unmentioned in the original report. The others all look like they are the result of filter improvements.

comment:31 @nacin2 years ago

In 24186:

Remove the body of the deprecated links_popup_script() as currently it issues a fatal error. see #24210.

comment:32 @nacin2 years ago

In 24187:

Remove manual printing of userSettings as utils.js receives this as inline script data (since 3.5). see #24210.

comment:33 @nacin2 years ago

In 24188:

remove_filter() only accepts three arguments: filter, callback, and priority. An accepted args parameter is only used for adds.

props rlerdorf.
see #24210.

comment:34 @nacin2 years ago

In 24189:

Fix usage of undeclared variables.

  • the_weekday_date() needs the global $currentday
  • ms_site_check() needs the global $current_site
  • media list table does not need to check for $total_orphans
  • upgrader has no $feedback variable, appears to be copypasta from other upgrade APIs
  • install_themes_feature_list() has no $features variable, return array() instead of a new return type of WP_Error

see #24210.

comment:35 @nacin22 months ago

Services_JSON was fixed upstream in http://svn.php.net/viewvc/pear/packages/Services_JSON/trunk/JSON.php?r1=330164&r2=330165&. Going to merge in that patch here as there have been some other changes since 1.0.3.

comment:36 @nacin22 months ago

In 24587:

Patch Services_JSON to use the proper function name and avoid a fatal error. see #24210.

comment:38 @nacin22 months ago

In 24590:

Use correct variable when calling request_filesystem_credentials() in delete_theme(). see #24210.

comment:39 @nacin22 months ago

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

Here's what is left: https://gist.github.com/nacin/5533549. With the exception of the WP filesystem ssh2 class — see #24277 — all of these are in external libraries. I'll be working to submit these upstream. Thanks again, rlerdorf.

comment:40 @nacin22 months ago

In 24591:

Use correct variable in the deprecated and abandoned Snoopy HTTP client. see #24210.

See also:

If any plugins are using this (instead of the WordPress HTTP API), this at least ensures that temp files are cleaned up.

Note: See TracTickets for help on using tickets.