Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 7 years ago

#19861 closed defect (bug) (fixed)

$wpdb->prepare() fails with localized floats

Reported by: laotse's profile laotse Owned by: nacin's profile nacin
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.3.1
Component: Database Keywords: has-patch
Focuses: Cc:

Description

The implementation of $wpdb->prepare() is buggy in several aspects. The mess shows strikingly, if you try to write floats using %f to the database using a server locale, which has a decimal colon instead of a dot.

Unfortunately sprintf() is localized in contrast to sccanf()! Furthermore, since PHP performs auto conversion, it can happen that a float is already passed as a string. Unfortunately, the array_walk() in prepare() escapes the ',' such that floatval() will drop the decimals. At least it does not produce another value, like if a float was passed.

I wrote a re-implementation, which also does without any '@' prefixes. It does well for the plugin - I did not yet try to replace the core function. I'd gladly provide my code to someone, who knows how to test the code thoroughly.

Attachments (8)

prepare.diff.bz2 (1.9 KB) - added by laotse 12 years ago.
19861.patch (695 bytes) - added by kurtpayne 12 years ago.
Alternate patch using %F to force localization unaware floats
19861.2.patch (1.6 KB) - added by SergeyBiryukov 12 years ago.
19861.3.patch (1.8 KB) - added by kurtpayne 12 years ago.
Do not stringify floats in real_escape
test_db.php (1.5 KB) - added by kurtpayne 12 years ago.
Unit test
19861.4.patch (921 bytes) - added by SergeyBiryukov 12 years ago.
19861.5.patch (796 bytes) - added by SergeyBiryukov 12 years ago.
19861-unit-test.patch (654 bytes) - added by kurtpayne 12 years ago.
Updated for lookbehind patch

Download all attachments as: .zip

Change History (29)

#1 follow-ups: @scribu
12 years ago

  • Component changed from General to Database

It would be useful if you could transform your code into a patch:

http://core.trac.wordpress.org/wiki#HowtoSubmitPatches

If not, just upload the modified wp-db.php file.

#2 in reply to: ↑ 1 @laotse
12 years ago

Replying to scribu:

It would be useful if you could transform your code into a patch:

Okay, don't have time this week, but I'll send a patch next week. Currently, I have a substitution function off the WP tree and the wpdb object.

#3 in reply to: ↑ 1 @laotse
12 years ago

I did convert my solution to a patch for the core function. However, there seem to be bugs in other parts of Wordpress, which pop up on this less tolerant solution. Obviously, some queries are generated with less format characters than arguments. Although I made my code somewhat more tolerant for this error, I still see various errors.

I read the instructions about how to set up a test environment and perform unit tests. Something I would be able to do, but currently lack the time. So if someone, who has this all set up and has experience in using it would apply the patch and tell me about the results, I'd gladly assist in hunting bugs.

Since I did not see any instructions as to where to upload my patch, I include it in this comment. The patch does 2 things. It defines a debugging function 'debug_log' in wp-config.php. This is just debugging and shall be removed later or be replaced by some canonical mechanism, if it exists. The other change is the replacement for wpdb::prepare().

diff --git a/wp-config.php b/wp-config.php
index 8a883bf..c24267d 100644
--- a/wp-config.php
+++ b/wp-config.php
@@ -111,7 +113,20 @@ define ('WPLANG', 'de_DE');
  * It is strongly recommended that plugin and theme developers use WP_DEBUG
  * in their development environments.
  */
-define('WP_DEBUG', false);
+define('WP_DEBUG', true);
+if(WP_DEBUG === true){
+       define('WP_DEBUG_DISPLAY', false);
+       define('WP_DEBUG_LOG', true);
+       function debug_log( $mMessage ){
+               if( is_array( $mMessage ) || is_object( $mMessage )){
+                       error_log( print_r( $mMessage, true ));
+               } else {
+                       error_log( $mMessage );
+               }
+       }
+} else {
+       function debug_log($mMessage) {}
+}
 
 /* That's all, stop editing! Happy blogging. */
 
diff --git a/wp-content/themes/tantra/header.php b/wp-content/themes/tantra/header.php
index 817c3e0..e3417e5 100644
--- a/wp-includes/wp-db.php
+++ b/wp-includes/wp-db.php
@@ -890,7 +890,7 @@ class wpdb {
         */
        function prepare( $query = null ) { // ( $query, *$args )
                if ( is_null( $query ) )
-                       return;
+                       return null;
 
                $args = func_get_args();
                array_shift( $args );
@@ -899,9 +899,92 @@ class wpdb {
                        $args = $args[0];
                $query = str_replace( "'%s'", '%s', $query ); // in case someone mistakenly already singlequoted it
                $query = str_replace( '"%s"', '%s', $query ); // doublequote unquoting
-               $query = preg_replace( '|(?<!%)%s|', "'%s'", $query ); // quote the strings, avoiding escaped strings like %%s
-               array_walk( $args, array( &$this, 'escape_by_ref' ) );
-               return @vsprintf( $query, $args );
+               $aParts = preg_split('/(%[^%])/', $query, null, PREG_SPLIT_DELIM_CAPTURE);
+               $sResult = "";
+               $pValue = null;
+               foreach ($aParts as $sPart){
+                       if(mb_strlen($sPart) != 2 || $sPart[0] != "%"){
+                               // literal string to copy
+                               $sResult .= $sPart;
+                       } else {
+                               if(count($args) <= 0){
+                                       // ran out of arguments
+                                       debug_log(__FILE__ . " (" . __LINE__ . ") wpdb::prepare(): provided more format codes than arguments!");
+                                       return false;
+                               }
+                               $mArg = array_shift($args);
+                               switch($sPart[1]) {
+                                       // %% cannot happen due to the delimiter pattern
+                                       default:
+                                               debug_log(__FILE__ . " (" . __LINE__ . ") wpdb::prepare(): unknown qualifier '".$sPart."'!");
+                                               // unknown qualifier
+                                               return false;
+                                       case 's':
+                                               if(!is_null($mArg) && !is_string($mArg)){
+                                                       return false;
+                                               }
+                                               $sResult .= (is_null($mArg))? 'NULL' : "'" . $this->_real_escape($mArg) . "'";
+                                               break;
+                                       case 'd':
+                                               if(!is_null($mArg) && is_string($mArg)){
+                                                       if(!preg_match('/^\d+$/',$mArg)){
+                                                               // string is not an integer representation
+                                                               debug_log(__FILE__ . " (" . __LINE__ . ") wpdb::prepare(): string '".$mArg."' is no integer!");
+                                                               return false;
+                                                       }
+                                                       $mArg = intval($mArg);
+                                               }
+                                               if(!is_null($mArg) && !is_int($mArg)){
+                                                       // parameter is not int
+                                                       debug_log(__FILE__ . " (" . __LINE__ . ") wpdb::prepare(): parameter is no integer!");
+                                                       return false;
+                                               }
+                                               $sResult .= (is_null($mArg))? 'NULL' : sprintf("%d",$mArg);
+                                               break;
+                                       case 'f':
+                                               if(!is_null($mArg) && is_string($mArg)){
+                                                       if(!is_string($pLValue)){
+                                                               // lazy initialization of a pattern for matching localized floats
+                                                               $aLocale = localeconv();
+                                                               $pSep = ($aLocale['mon_thousands_sep'] == '.')? '\.' : $aLocale['mon_thousands_sep'];
+                                                               $pDot = ($aLocale['mon_decimal_point'] == '.')? '\.' : $aLocale['mon_decimal_point'];
+                                                               $pLValue = '/^(:?\d+(:?'.$pSep.'\d+)*(:?'.$pDot.'\d*)?|'.$pDot.'\d+)$/';
+                                                       }
+                                                       if(preg_match($pLValue,$mArg)){
+                                                               // looks like a server locale formatted float string
+                                                               // we have $aLocale, since we have $pLValue!
+                                                               $mArg = str_replace($aLocale['mon_thousands_sep'], '', $mArg);
+                                                               $mArg = str_replace($aLocale['mon_decimal_point'], '.', $mArg);
+                                                               $mArg = floatval($mArg);
+                                                       } elseif(preg_match('/^(:?\d+(:?\.\d*)|\.\d+)$/',$mArg)){
+                                                               // looks like an internal float string
+                                                               $mArg = floatval($mArg);
+                                                       } else {
+                                                               // looks pretty unknown
+                                                               debug_log(__FILE__ . " (" . __LINE__ . ") wpdb::prepare(): string '".$mArg."' is no float!");
+                                                               return false;
+                                                       }
+                                               }
+                                               if(!is_null($mArg) && !is_float($mArg)){
+                                                       // this is no float
+                                                       debug_log(__FILE__ . " (" . __LINE__ . ") wpdb::prepare(): parameter is no float!");
+                                                       return false;
+                                               }
+                                               // SQL necessarily requires a decimal dot and no 1000 seps
+                                               // %F is expected to provide this format
+                                               $sResult .= (is_null($mArg))? 'NULL' : sprintf("%F",$mArg);
+                                               break;
+                               } // end switch(qualifier)
+                       } // end if(string or qualifier)
+               } // end foreach
+               // check consistence
+               if(count($args) > 0){
+                       // more arguments than format codes
+                       debug_log(__FILE__ . " (" . __LINE__ . ") wpdb::prepare(): provided less format codes than arguments in query: '".$query."'");
+                       // should strictly be false, but some core functions are broken!
+                       return $sResult;
+               }
+               return $sResult;
        }
 
        /**

#4 @scribu
12 years ago

You should attach the patch as a file, instead of pasting it in the comment box.

@laotse
12 years ago

#5 @coffee2code
12 years ago

  • Version set to 3.3.1

You should also:

  • Attach the .diff directly (and not compressed)
  • If using git to generate the diff, (from the WP root) do: git diff --no-prefix > 19861.diff
  • Ideally, as done in my previous example, use the ticket number as part of the diff filename

I would also recommend separating out your debugging-related changes and attaching that code/diff separately.

And based on the nature of the changes you're making, you're really going to want to include some unit tests if you hope to get some traction (see http://unit-test.trac.wordpress.org/). Even if not a true unit test patch, at least some example code to demonstrate the issues as things stand now, and which would work once your patch is applied.

#6 @SergeyBiryukov
12 years ago

  • Keywords needs-patch needs-unit-tests added

#7 @kurtpayne
12 years ago

  • Cc kpayne@… added

This patch looks like a re-implementation of vsprintf, but with more validation. Since vsprintf already does sanitization, though, why do we need the extra regexes in this page?

The original issue in the ticket was regarding in correct parsing of localized floats. You mention %F in your patch, why not just use %F instead of %f and skip the extra work?

@kurtpayne
12 years ago

Alternate patch using %F to force localization unaware floats

#8 @scribu
12 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.5

Marked #21045 as dup.

We should go with 19861.patch.

#9 @SergeyBiryukov
12 years ago

To summarize, this is the snippet (based on #21045) that I've tested the issue with:

setlocale( LC_ALL, array( 'ru_RU.utf8', 'rus' ) );
$wpdb->update( 
	'test_table', 
	array( 'float_column' => 0.7 ), 
	array( 'meta_id' => 5 ), 
	array( '%f' ), 
	array( '%d' ) 
);

This results in the error:

WordPress database error: [You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '000000 WHERE `meta_id` = 5' at line 1]
UPDATE `test_table` SET `float_column` = 0,000000 WHERE `meta_id` = 5

With 19861.patch, there's no error, but the value passed to $wpdb->prepare() is still incorrect (0,7 instead of 0.7), so it's not converted properly. The resulting query is:

UPDATE `test_table` SET `float_column` = 0.000000 WHERE `meta_id` = 5

19861.2.patch is an attempt to fix that. The proper query would be:

UPDATE `test_table` SET `float_column` = 0.700000 WHERE `meta_id` = 5

#10 @knutsp
12 years ago

  • Cc knut@… added

@kurtpayne
12 years ago

Do not stringify floats in real_escape

@kurtpayne
12 years ago

Unit test

#11 follow-up: @kurtpayne
12 years ago

  • Keywords needs-unit-tests removed

The patch to prepare didn't fully work. The $args array was passed through escape_by_ref which stringified each element. vsprintf will not properly convert (string) 0,7 to (string) 0.7. It will only work with (float) 0,7. 19861.3.patch addresses this.

Also added a unit test.

#12 in reply to: ↑ 11 @SergeyBiryukov
12 years ago

With the check in escape_by_ref(), seems that string replacements from 19861.2.patch are no longer needed.

#13 @stephenh1988
12 years ago

  • Cc contact@… added

#14 @kurtpayne
12 years ago

Unit test committed in [UT746]

#15 @nacin
12 years ago

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

In [21161]:

Handle localized floats in $wpdb->prepare(). props kurtpayne. fixes #19861.

#16 follow-up: @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Could a query string with "%f" -- not meant as a float replacement -- be broken in this regard? Should we do a lookbehind like the line after it to make sure we're not dealing with an escaped %%f?

#17 in reply to: ↑ 16 @SergeyBiryukov
12 years ago

Replying to nacin:

Could a query string with "%f" -- not meant as a float replacement -- be broken in this regard?

Yes, an escaped %%f is currently altered as well. 19861.5.patch adds a lookbehind.

@kurtpayne
12 years ago

Updated for lookbehind patch

#18 @kurtpayne
12 years ago

Updated unit test to match 19861.5.patch. The patch looks good.

#20 @nacin
11 years ago

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

In [22304]:

When replacing floats in wpdb::prepare(), avoid escaped placeholders (%%f). props SergeyBiryukov. fixes #19861.

#21 @johnbillion
7 years ago

In 40528:

Build/Test Tools: Add some locale debugging to the Travis config so we can determine which locales are available to test with.

See #40533, #19861

Note: See TracTickets for help on using tickets.