Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#27982 closed defect (bug) (fixed)

Stop passing boolean values to mysqli_fetch_object()

Reported by: markjaquith's profile markjaquith Owned by: markjaquith's profile markjaquith
Milestone: 4.1 Priority: normal
Severity: normal Version: 3.9
Component: Database Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

Our call to mysqli_fetch_object() in wp-db.php sometimes receives a $result that is a boolean.

This makes our unit tests blow up in HHVM, as their implementation of mysqli_fetch_object() isn't as forgiving when receiving an unexpected value.

A simple fix is to do an is_object() test before the call. This lets our unit tests run in HHVM. But also I'd like to know why $result has a boolean value.

Attachments (2)

27982.diff (527 bytes) - added by pento 10 years ago.
27982.2.diff (1.1 KB) - added by michalzuber 10 years ago.

Download all attachments as: .zip

Change History (20)

@pento
10 years ago

#1 @pento
10 years ago

  • Keywords has-patch 2nd-opinion added
  • Milestone changed from Awaiting Review to 3.9.1
  • Owner set to pento
  • Status changed from new to accepted

I've had a bit of a dig through HHVM/PHP, and it seems there might be some cases (with my limited knowledge of how the PHP internals work) where mysqli_query() can return false without setting an error. So, the check for an error in wpdb::query() doesn't catch it.

With attachment:27982.diff, wpdb::print_error() will record the error, but with an empty error string - I'm not wild about this behaviour, perhaps it should have a default error string, so we're at least recording something?

#2 @nacin
10 years ago

Is this something specific to mysqli? Is there a concern this could happen with ext/mysql? I'd assume we would have noticed this, but it's also error-suppressed.

#3 @pento
10 years ago

On further reflection, I'm not entirely sure why it isn't being caught by our existing query-type handling.

Mark, are there any particular unit tests that are consistently breaking here?

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

#5 @markjaquith
10 years ago

Our query type handling misses "SET" queries. Those hit the else branch. Those have a boolean result. We then try to pass that boolean to mysqli_fetch_object().

I think we can safely skip the while loop if is_bool( $this->result ). Passing a boolean to `mysqli_fetch_object() is bad behavior.

#6 @nacin
10 years ago

  • Owner changed from pento to markjaquith
  • Status changed from accepted to assigned

#7 @nacin
10 years ago

  • Milestone changed from 3.9.1 to 3.9.2

#8 @DrewAPicture
10 years ago

@markjaquith: Do you still want to push this in for 4.0/3.9.2 or punt?

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

#9 @michalzuber
10 years ago

Couldn't find boolean result source, worth mentioning I'm not a C guru :)
Looked at:

According to me from PHP source it shouldn't return boolean only if it's array
if (into_object && Z_TYPE_P(return_value) == IS_ARRAY) ext/mysqli/mysqli.c#L1285

Checked why might mysql_fetch_object be error suppressed wp-includes/wp-db.php#L1617 and
found that the initial class author https://github.com/WordPress/WordPress/blob/43e4a8a930be8b3d0359e517e4a6ef12f0d0ba73/wp-includes/wp-db.php#L203 had it.

Yeah, I'm aware that the following isn't the "nicest", but better then it is:

if ( $this->use_mysqli ) {
	if ( WP_DEBUG ) {
		while ( $row = mysqli_fetch_object( $this->result ) ) {
			$this->last_result[$num_rows] = $row;
			$num_rows++;
		}
	} else {
		while ( $row = @mysqli_fetch_object( $this->result ) ) {
			$this->last_result[$num_rows] = $row;
			$num_rows++;
		}
	}
} else {
	if ( WP_DEBUG ) {
		while ( $row = mysql_fetch_object( $this->result ) ) {
			$this->last_result[$num_rows] = $row;
			$num_rows++;
		}
	} else {
		while ( $row = @mysql_fetch_object( $this->result ) ) {
			$this->last_result[$num_rows] = $row;
			$num_rows++;
		}
	}
}
Version 0, edited 10 years ago by michalzuber (next)

@michalzuber
10 years ago

#10 @michalzuber
10 years ago

$this->result is set at line 1652

According to http://php.net/manual/en/mysqli.query.php there are boolean value situations:
Returns FALSE on failure. For successful SELECT, SHOW, DESCRIBE or EXPLAIN queries mysqli_query() will return a mysqli_result object. For other successful queries mysqli_query() will return TRUE.

#11 @markjaquith
10 years ago

  • Milestone changed from 3.9.2 to Future Release

Towards getting 4.0 out the door, I'm punting this and will take another look for 4.1.

#12 @SergeyBiryukov
10 years ago

#30555 was marked as a duplicate.

#13 @petermantos
10 years ago

wp-db.php has a bug around line 1616 (most recent version as of 11/30/2014)
They error reported is:
Debug Warning: ...\wp-includes\wp-db.php line 1622
mysql_fetch_object(): supplied argument is not a valid MySQL result resource
A similar comes up under different circumstances that basically states that the result is a boolean, not an object.
The solution is to make a test as to whether or not the result is an object using is_object as follows:
$num_rows = 0;
if ( is_object($this->result ) ) {
if ( $this->use_mysqli ) {
while ( $row = @mysqli_fetch_object( $this->result ) ) {
$this->last_result[$num_rows] = $row;
$num_rows++;
}
} else {
while ( $row = @mysql_fetch_object( $this->result ) ) {
$this->last_result[$num_rows] = $row;
$num_rows++;
}
}
}

#14 @pento
10 years ago

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

In 30677:

WPDB: Before fetching results, make sure we have a valid result resource to fetch them from.

Fixes #27982.

#15 @rmccue
10 years ago

  • Milestone changed from Future Release to 4.1

#16 @pento
10 years ago

#30477 was marked as a duplicate.

#17 @pento
10 years ago

#30595 was marked as a duplicate.

#18 @mark8barnes
10 years ago

I just came to report this error when using the SET command. Glad to see it's fixed in 4.1, and was able to patch 4.0.1 with the fix, too.

Note: See TracTickets for help on using tickets.