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: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (9)
#2
follow-up:
↓ 4
@
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
@
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
@
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
is1
,$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
@
6 years ago
- Owner set to dd32
- Resolution set to fixed
- Status changed from new to closed
In 42226:
Thanks @natacado for reporting this, especially with the added bonus of the unit test!
Moving to 4.9.1 for review.