WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 9 months ago

Last modified 9 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 12 months ago.
24210.2.patch (6.8 KB) - added by SergeyBiryukov 12 months ago.
24210.wpdb.patch (1.9 KB) - added by SergeyBiryukov 12 months ago.

Download all attachments as: .zip

Change History (43)

comment:1 follow-up: rlerdorf12 months ago

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

comment:2 SergeyBiryukov12 months ago

  • Description modified (diff)

comment:3 SergeyBiryukov12 months ago

In 24118:

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

comment:4 SergeyBiryukov12 months ago

  • Milestone changed from Awaiting Review to 3.6

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

SergeyBiryukov12 months ago

SergeyBiryukov12 months ago

comment:5 SergeyBiryukov12 months 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 12 months ago by SergeyBiryukov (previous) (diff)

comment:6 follow-up: SergeyBiryukov12 months 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 SergeyBiryukov12 months 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 12 months ago by SergeyBiryukov (previous) (diff)

comment:8 in reply to: ↑ 6 ocean9012 months 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.

comment:9 SergeyBiryukov12 months ago

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

comment:10 SergeyBiryukov12 months ago

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

comment:11 SergeyBiryukov12 months 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 0, edited 12 months ago by SergeyBiryukov (next)

comment:12 SergeyBiryukov12 months 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 SergeyBiryukov12 months 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 SergeyBiryukov12 months ago

In 24120:

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

comment:15 follow-up: SergeyBiryukov12 months ago

In 24121:

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

comment:16 SergeyBiryukov12 months ago

In 24122:

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

comment:17 SergeyBiryukov12 months ago

In 24123:

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

comment:18 SergeyBiryukov12 months ago

  • Description modified (diff)

comment:19 in reply to: ↑ 1 SergeyBiryukov12 months 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 rlerdorf12 months 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: dd3212 months 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 nacin12 months ago

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

comment:23 in reply to: ↑ 21 SergeyBiryukov12 months 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 nacin12 months ago

In 24125:

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

comment:25 nacin12 months 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 nacin12 months 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 nacin12 months ago

In 24128:

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

props rlerdorf.
see #24210.

comment:28 nacin12 months 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: rlerdorf11 months 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 11 months ago by rlerdorf (previous) (diff)

comment:30 in reply to: ↑ 29 nacin11 months 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 nacin11 months ago

In 24186:

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

comment:32 nacin11 months ago

In 24187:

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

comment:33 nacin11 months 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 nacin11 months 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 nacin9 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 nacin9 months ago

In 24587:

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

comment:38 nacin9 months ago

In 24590:

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

comment:39 nacin9 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 nacin9 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.