Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#37660 closed defect (bug) (fixed)

RegEx in wpdb::get_table_from_query( $q ) in wp-db.php returns table_alias='a' rather than table_name=wp_999_options

Reported by: webp's profile webp Owned by: pento's profile pento
Milestone: 4.7 Priority: normal
Severity: normal Version: 4.1.2
Component: Database Keywords: needs-unit-tests needs-patch
Focuses: Cc:

Description

WordPress database error

WordPress database error Table 'wp_xyz.wp_999_options' doesn't exist for query
    DELETE a, b FROM wp_999_options a, wp_999_options b
    WHERE a.option_name LIKE '\\_transient\\_%'
    AND a.option_name NOT LIKE '\\_transient\\_timeout\\_%'
    AND b.option_name = CONCAT( '_transient_timeout_', SUBSTRING( a.option_name, 12 ) )
    AND b.option_value < 1470982769 made by populate_options

Problem

function populate_options() in wp-admin/includes/schema.php fires the SQL query above that goes through hyperdb and then $wpdb in wp-db.php.

class hyperdb extends wpdb in wp-includes/wp-db.php:

# hyperdb/db.php

class hyperdb extends wpdb {
  function get_table_from_query( $q ) {
    if( method_exists( get_parent_class( $this ), 'get_table_from_query' ) ) {
      // WPDB has added support for get_table_from_query, which should take precedence
      return parent::get_table_from_query( $q );
    }

# wp-includes/wp-db.php

  protected function get_table_from_query( $query ) {
    // Quickly match most common queries.
    if ( preg_match( '/^\s*(?:'
        . 'SELECT.*?\s+FROM'
        . '|INSERT(?:\s+LOW_PRIORITY|\s+DELAYED|\s+HIGH_PRIORITY)?(?:\s+IGNORE)?(?:\s+INTO)?'
        . '|REPLACE(?:\s+LOW_PRIORITY|\s+DELAYED)?(?:\s+INTO)?'
        . '|UPDATE(?:\s+LOW_PRIORITY)?(?:\s+IGNORE)?'
        . '|DELETE(?:\s+LOW_PRIORITY|\s+QUICK|\s+IGNORE)*(?:\s+FROM)?'
        . ')\s+((?:[0-9a-zA-Z$_.`-]|[\xC2-\xDF][\x80-\xBF])+)/is', $query, $maybe ) ) {
      return str_replace( '`', '', $maybe[1] );
    }

Cause

$query = DELETE a, b FROM wp_999_options a, wp_999_options b ....

RegEx in wpdb::get_table_from_query( $q ) in wp-db.php below returns table_alias='a' rather than table_name=wp_999_options from $query above:

. '|DELETE(?:\s+LOW_PRIORITY|\s+QUICK|\s+IGNORE)*(?:\s+FROM)?'

WordPress database error occurs when Table wp_999_options is not found in default database wp_xyz. So my hyperdb config directs unknown db name 'a' to default db wp_xyz, rather than my hyperdb mapped database wp_abc.

Suggested Fix

Change RegEx to:
. '|DELETE(?:.+FROM)?'

Is it a good fix? Or am I missing other scenarios?

Core Developers should also watch out if other similar SQL might failed in wpdb::get_table_from_query( $q ):

SQL COMMAND table_alias FROM table, table_alias

If fix confirmed, someone should ask hyperdb to fix as similar RegEx is used. Not too many WordPress developers use hyperdb so I'm too lazy to submit a ticket for hyperdb myself.
But wpdb is used everywhere and it should be perfect, or strive to be perfect. Otherwise, developers like me will be scratching my head for hours when minor wpdb errors arise. Even though this bug is minor, but unknown problems might cause database corruption months down the line, at least for me and my massive number of replicated databases and servers.

Change History (3)

#1 @pento
8 years ago

  • Keywords needs-unit-tests needs-patch added
  • Milestone changed from Awaiting Review to 4.7
  • Owner set to pento
  • Status changed from new to accepted
  • Version changed from trunk to 4.1.2

Thank you for the detailed investigation and bug report, @webp!

You are correct, this is definitely a bug, even more so that it affects a query in Core.

I think the fix will be to replace the current (?:\s+FROM) part of the match with (?:.+?FROM) - similar to your suggestion, but the additional ? should make it a non-greedy match, stopping at the first FROM. This will need testing, of course. :-)

The obvious shortcoming of wpdb::get_table_from_query() is that it only returns the first table in a multi-table query - as was the case with the method's HyperDB origins, it assumes that you're only querying tables on the same server. I'm fine with this behaviour continuing for this variation of DELETE syntax.

I had a quick look at the other queries that wpdb::get_table_from_query() matches - I don't think there are any affected by similar bugs. Extra eyes are always welcome, of course, please point out if you notice any others!

#2 @jorbin
8 years ago

@pento I think this might be something worth getting in on the early side. What kind of unit tests would you recommend someone write for this?

#3 @pento
8 years ago

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

In 38507:

Database: Find the correct table names in DELETE queries with table aliases

Previously, wpdb::get_table_from_query() would not find the correct table name in the query DELETE a FROM table a, due to not recognising the table alias immediately after the DELETE as correct syntax.

Fixes #37660.

Note: See TracTickets for help on using tickets.