Regression: Wordpress 4.9 does not parse DB_HOST socket paths with colons correctly

[natacado]

The new parse_db_host code introduced in Wordpress 4.9 to parse IPv6 database host information has caused a regression: if connecting to a database using a UNIX socket and the socket path contains a colon, it fails to parse because it assumes it's an IPv6 address by virtue of having more than one colon.

This is common for users of Google Cloud SQL using the cloud_sql_proxy; it creates sockets with paths of the form /cloudsql/$PROJECT:$REGION:$DATABASE (​https://cloud.google.com/sql/docs/mysql/connect-external-app - see step 7, "UNIX SOCKETS" tab.)

You can reproduce this by adding an additional unit test:

Index: tests/phpunit/tests/db.php
===================================================================
--- tests/phpunit/tests/db.php	(revision 42204)
+++ tests/phpunit/tests/db.php	(working copy)
@@ -1559,6 +1559,14 @@
 				false,
 			),
 			array(
+				':/tmp/mysql:with_colon.sock',
+				false,
+				'',
+				null,
+				'/tmp/mysql:with_colon.sock',
+				false,
+			),
+			array(
 				'127.0.0.1',
 				false,
 				'127.0.0.1',

[dd32]

Thanks @natacado for reporting this, especially with the added bonus of the unit test!

Moving to 4.9.1 for review.

[natacado]

Socket parsing fix

[natacado]

I attached a proposed patch that fixes it for me. It's my first patch for Wordpress, be gentle. :-) This patch depends on the fact that a socket path will always be absolute so we can search for :/ in the string; the old code seemed to already assume this for IPv6 but didn't do so in IPv4 code, so ... maybe there's someone out there using something like

define('DB_HOST', 'localhost:mysql.sock');

And relying on some poorly-documented assumptions about current working directory for where mysql.sock would exist? I don't think it's a good idea to have something like this, but this patch would regress that.

The other idea I had was to use inet_pton to properly search for/parse the IP address component from the start of the string, but that's not supported on Windows until PHP 5.3, so probably off-limits in Wordpress core.

[dd32]

Thanks for the patch @natacado

I don't see an issue with assuming that socket paths are absolute, I can't really see why someone would use a relative one (or if it even works)

[dd32]

Replying to natacado:

This patch depends on the fact that a socket path will always be absolute so we can search for :/ in the string; the old code seemed to already assume this for IPv6 but didn't do so in IPv4 code, so ... maybe there's someone out there using something like

It looks like that was actually a bug with the parsing of the hostname, as it was incorrectly stripping the / from IPv6 socket names, so it can't have worked anyway :)

I don't actually think many of the cases below would ever be actually used, but documenting here the weirdness of the edge cases which came up:

• ::1:/tmp/sock ($host is ::1:, $socket is 'tmp/sock')
• [::1]:/tmp/sock ($host is ::1:, $socket is null)
• 1.2.3.4:1234:/tmp/sock ($socket is null, $host is 1, $port is null)

The patch makes them all parse "correctly", I've added some unit tests too. So I'm going to commit this to trunk with the hope that @pento can review it before I merge it to 4.9.1.

[dd32]

In 42226:

WPDB: Fix the parsing of sockets which contain colons within the socket name (used on some cloud providers).

Props natacado.
Fixes #42634 for trunk.

[dd32]

reopening for 4.9.1 after review and trunk soak.

[pento]

Looks good to me!

[dd32]

In 42229:

WPDB: Fix the parsing of sockets which contain colons within the socket name (used on some cloud providers).

Props natacado.
Merges [42226] to the 4.9 branch.
Fixes #42634 for 4.9.