$wpdb->prepare() fails with localized floats

[laotse]

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.

[scribu]

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.

[laotse]

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.

[laotse]

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;
        }
 
        /**

[scribu]

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

[coffee2code]

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.

[kurtpayne]

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]

Alternate patch using %F to force localization unaware floats

[scribu]

Marked #21045 as dup.

We should go with 19861.patch​.

[SergeyBiryukov]

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

[kurtpayne]

Do not stringify floats in real_escape

[kurtpayne]

Unit test

[kurtpayne]

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.

[SergeyBiryukov]

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

[kurtpayne]

Unit test committed in ​[UT746]

[nacin]

In [21161]:

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

[nacin]

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?

[SergeyBiryukov]

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]

Updated for lookbehind patch

[kurtpayne]

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

[nacin]

​[UT1093]

[nacin]

In [22304]:

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

[johnbillion]

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