Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#41944 new enhancement

Add %u support to wpdb->prepare

Reported by: charlestonsw's profile charlestonsw Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8.2
Component: Database Keywords:
Focuses: Cc:

Description

MySQL unsigned int: 4bn
PHP %d signed int: 2bn

Maybe the standard should be %u not %d or at the very least support both while internally using %u for all key lookups?

Especially since much of the WP Core specifically sets ID fields as bigint unsigned not null auto_increment.

If a site never has more than 2bn records, and MOST will NEVER get that big, it is a non-issue but shouldn't the standard be consistent between the DB engine and the language processor (PHP)?

Change History (7)

#1 @soulseekah
7 years ago

There is no unsigned type in PHP. http://php.net/manual/en/language.types.integer.php

Moreover %u cannot replace %d as negative numbers are valid data.

PHP_MAX_INT on 64-bit systems is 9223372036854775807. In order to pass larger number there are special libraries or the bcmath, GMP extensions. These could be passed in as strings to MySQL for the unsigned column types.

So %u wouldn't really help.

Last edited 7 years ago by soulseekah (previous) (diff)

#2 @charlestonsw
7 years ago

That may be true, but how does sprintf work in PHP? They document it as though %d and %u are NOT the same.

http://php.net/manual/en/function.sprintf.php

d - the argument is treated as an integer, and presented as a (signed) decimal number.
u - the argument is treated as an integer, and presented as an unsigned decimal number.

And since there are many instances where you are looking up a KEY which can be a MySQL unsigned int doing a select * where key=%u seems prudent. Unless someone can confirm that PHP sprintf engine allows %d to render a full unsigned int. I'm not familiar with the internal workings of PHP but the public docs indicate %d and %u are different.

I'm not saying "replace %d with %u" , BTW, just saying %u should ALSO be supported.

Last edited 7 years ago by charlestonsw (previous) (diff)

#3 follow-up: @charlestonsw
7 years ago

sprintf( 'Unsigned %u Signed %d <br/>' , PHP_INT_MAX , PHP_INT_MAX )

Unsigned 9223372036854775807 Signed 9223372036854775807

Apparently this is a non-issue other than the fact that WP 4.8.2 broke a lot of code that used %u instead of %d while gaining no advantage with regard to the original security concerns the patch was meant to address.

After all active plugins replace any %u referenced with %d this will be a non-issue but maybe it should be considered to keep those handful of plugins that are using it from breaking because someone updates WP to version 4.8.2+.

#4 @soulseekah
7 years ago

Again, there is no unsigned int in PHP.

The %u is there to convert signed integers to unsigned ones for various lower-level (think binary protocols mostly) purposes.

Observe the following behavior:

> printf( '%u', -1 );
18446744073709551615

See how we got a number printed that is twice as large as PHP_MAX_INT? But you can never store that in a variable of type <code>integer</code>:

> echo (int)sprintf( '%u', -1 );
9223372036854775807

Which is just an overflow. There no unsigned integer type in PHP, hope this makes sense. Another overflow observation:

> echo 18446744073709551615;
1.844674407371E+19
> echo gettype( 18446744073709551615 );
double

But back to unsigned ints. So what you would have to do to make it sort of work is to sent a negative number to the prepare function: <code>$wpdb->prepare( "%u", -595231 )</code> to send a value larger than PHP_MAX_INT to the database. Even then, the prepare method is simply converting everything to a string. So you're not actually sending a number over the line but a string.

So in order to make it work, you'd need some sort of function that converts a numeric string that is larger than PHP_MAX_INT into a signed counterpart that is smaller than (PHP_MAX_INT * 2) and sends a negative number into the prepare method just to get a string back that you send to the database. Why not just sent the numeric string into an %s placeholder to begin with?

The same with GMP/bcmath libraries.

So I invite you to show me a snippet of code that gets a large (> PHP_MAX_INT) numeric value from say <code>$_GETid?</code> and does a <code>SELECT * FROM wp_posts WHERE ID = %u</code> on it.

$post_id = $_GET['id'];
$wpdb->query( $wpdb->prepare( 'SELECT * FROM wp_posts WHERE ID = %u', $post_id ) );

Type coercion in PHP will try to set it as int, observe:

> printf( '%u', '18446744073709551615' );
9223372036854775807

Overflow. Any ideas?

Version 1, edited 7 years ago by soulseekah (previous) (next) (diff)

#5 in reply to: ↑ 3 @soulseekah
7 years ago

Replying to charlestonsw:

Apparently this is a non-issue other than the fact that WP 4.8.2 broke a lot of code that used %u instead of %d while gaining no advantage with regard to the original security concerns the patch was meant to address.

After all active plugins replace any %u referenced with %d this will be a non-issue but maybe it should be considered to keep those handful of plugins that are using it from breaking because someone updates WP to version 4.8.2+.

Oh, I see. So you had code that actually used %u?

Well I'm being met with a lot of hurdles after requesting numbered placeholders #41925

So you're seem to be in the same boat. But if you can show that a lot of code used %u out there (because a lot of code used numbered placeholders) you might be able to push for %u coming back for back-compatibility reasons. It's simpler than numbered placeholders, but from a practical point of view the %u is completely %useless :)

Last edited 7 years ago by SergeyBiryukov (previous) (diff)

#6 @charlestonsw
7 years ago

Not sure how many other use %u. WP add ons (plugins, themes, etc.) is a big codebase to assume nobody else uses that format. Remember, not all WP related code lives in the WP plugin/theme repo so you'll never really know what else was broken.

wpdb->prepare %u support was dropped because it wasn't in the format notes. Dropping that support broke things. It was not a change that made WP more secure but did make it less functional for some users; at least 5,000 installed Power add on users in my case.

Not a lot of code references to "up the tally" for %u but how to we value the change? By customers affected or by line-counts of code?

#7 @soulseekah
7 years ago

Well 1.2 million lines of code, according to GitHub search https://github.com/search?q=wpdb-%3Eprepare+%251%24s&type=Code&utf8=%E2%9C%93 broke on usage of numbered placeholders (I and didn't even look for variations of the placeholders, including Yoast, Wordfence, some of Automattic's stuff). But as you can see my ticket was closed and wontfix.

I wrote some thoughts about this here https://codeseekah.com/2017/09/21/on-wordpress-security-and-contributing/

And indeed, %u is has nothing to do with the security concern. But the official stance of the higher-ups seems to be that if it's wasn't supported and documented in the code then it's your problem, it won't be fixed.

My strong belief is that if it is useful, if it was used, the possibility of adding it for that sake should not be discounted so strongly.

Placeholders - useful. Used? - a freaking lot! Should be considered - definitely.

%u - not useful. Used? - I guess so, otherwise this ticket would not be here. Should be considered - hell yes.

Last edited 7 years ago by soulseekah (previous) (diff)
Note: See TracTickets for help on using tickets.