Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#42634 closed defect (bug) (fixed)

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

Reported by: natacado's profile natacado Owned by: dd32's profile dd32
Milestone: 4.9.1 Priority: normal
Severity: normal Version: 4.9
Component: Database Keywords: has-patch fixed-major commit
Focuses: Cc:

Description

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',

Attachments (1)

42634-fix-socket-parsing-ipv6.diff (1.7 KB) - added by natacado 6 years ago.
Socket parsing fix

Download all attachments as: .zip

Change History (9)

#1 @dd32
6 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.9.1

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

Moving to 4.9.1 for review.

@natacado
6 years ago

Socket parsing fix

#2 follow-up: @natacado
6 years ago

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.

#3 @dd32
6 years ago

  • Keywords has-patch added; needs-patch removed

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)

#4 in reply to: ↑ 2 @dd32
6 years ago

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.

#5 @dd32
6 years ago

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

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.

#6 @dd32
6 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

reopening for 4.9.1 after review and trunk soak.

#7 @pento
6 years ago

  • Keywords commit added

Looks good to me!

#8 @dd32
6 years ago

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

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.

Note: See TracTickets for help on using tickets.