Occasional PHP exception being thrown on WPDB/MySQLi connections

[johnjamesjacoby]

PHP Debug Notice

mysqli::real_connect() expects parameter 5 to be integer, string given - wpdb::_do_query /wp-includes/wp-db.php:2056

Parameter 5 is $port which in my case is not explicitly defined in DB_HOST.

I have a hunch it's happening on reconnection attempts (using $this->dbh) but don't have any hard evidence to back up that claim.

The above notice appears in logs once for approximately every 250k requests.

My DB_HOST is localhost, and I've also seen this happen with RDS (xxx.rds.amazonaws.com).

---

I've spent a few hours researching this, and am including that research below:

WordPress DB_HOST Documentation

• ​https://wordpress.org/support/article/editing-wp-config-php/#set-database-host

The above docs confirm that localhost:3306 is the recommended way of adding a port number.

CodeIgniter example & fix

• ​https://github.com/bcit-ci/CodeIgniter/issues/5627
• ​https://github.com/bcit-ci/CodeIgniter/commit/41bba2fea6fafa315db0a21093722f920455e3b1

The above links confirm that CodeIgniter was defaulting to a string (causing the same issue) instead of null or int, and patched it internally.

Facebook, HHVM, HACK

• ​https://github.com/facebook/hhvm/issues/2482
• ​https://github.com/facebook/hhvm/commit/f03f434e2eacda604b32bc7e4aa3334ed956a83a

The above issue confirms that Facebook saw the same issue, and decided to patch it inside of HHVM.

Tangential WordPress tickets

• #41722 - Adding IPv6 support
• #42496 - Improving MySQL 8.0 support
• #21663 - Adding mysqli support

The above tickets show an abbreviated timeline of surrounding changes to wpdb. No regressions. No bug introductions. Simply some related research.

PHP MySQL unit tests

• ​https://github.com/php/pecl-database-mysql/blob/6ca4fa4b509819ecc4c8ff2b28080d60ad64acc6/tests/mysql_connect.phpt

The above link shows how PHP itself tests MySQL (not MySQLi) database connections.

PHP MySQLi Docs

• ​https://www.php.net/manual/en/class.mysqli.php

The above link is my favorite. Specifically:

class mysqli {
    (...)
    public __construct(
        string $hostname = ini_get("mysqli.default_host"),
        string $username = ini_get("mysqli.default_user"),
        string $password = ini_get("mysqli.default_pw"),
        string $database = "",
        int $port = ini_get("mysqli.default_port"),
        string $socket = ini_get("mysqli.default_socket")
    )
    (...)
    public real_connect(
        string $host = ?,
        string $username = ?,
        string $passwd = ?,
        string $dbname = ?,
        int $port = ?,
        string $socket = ?,
        int $flags = ?
    ): bool
}

ini_get() always returns a string, and PHP's mysqli class constructs & defaults its $port using it. In the PHP source, it does not appear to be type cast into an integer.

---

Hypothesis

I believe this small and random-feeling annoyance can be fixed quite easily in WordPress in much the same way it's been fixed in HHVM & CodeIgniter.

I have a hunch that: web hosts silence or ignore these notices; they've been happening for a very long time; the problem is small enough where it wasn't worth anyone else digging too deep into.

---

Conclusion

• wpdb::parse_db_host() parses $this->dbhost looking for IPv4/IPv6, port number, sockets, etc...
• $this->dbhost is gotten from DB_HOST
• Casting $port to (int) when non-null will ensure mysqli_real_connect() will only ever see null or int type values

Something like:

$port = $port ? absint( $port ) : null;

[johnjamesjacoby]

54877.patch proposes the following minimum change:

After parsing and before returning:

• When $port is truthy, return absint( $port )
• When $port is falsy, return null

---

A more comprehensive change could refactor the following code, removing the foreach loop & removing the only variable-variable ($$component) in WordPress:

		foreach ( array( 'host', 'port' ) as $component ) {
			if ( ! empty( $matches[ $component ] ) ) {
				$$component = $matches[ $component ];
			}
		}

[azouamauriac]

hello @johnjamesjacoby your patch looks good, though i've a suggestion, absint always returns positive interger, but mysqli_real_connect function just need interger(no matter the sign) as port paramerter. I think we should just convert $port into int(no matter the sign). What do you think about that?

[johnjamesjacoby]

Hi @azouamauriac 👋

absint() is a tricky thing. It’s a good idea to prevent invalid values, but it’s a bad idea to silently flip a negative user-supplied number into a positive one.

In this case, negative port numbers do not exist (and 0 is technically a “protected” port ID that no one should use) hence I went with absint() over (int) to keep MySQLi happy (preventing an error) even if an invalid (negative) port was attempted.

The ! empty() check above my code change effectively avoids a 0 value (or non-existent) port ID, but nothing checks for and avoids a negative numeric value.

I consider absint() here to be a tiny bit of defensive application protection, and I remain very open to alternative approaches and perspectives 👐

[SergeyBiryukov]

Replying to johnjamesjacoby:

A more comprehensive change could refactor the following code, removing the foreach loop & removing the only variable-variable ($$component) in WordPress:

		foreach ( array( 'host', 'port' ) as $component ) {
			if ( ! empty( $matches[ $component ] ) ) {
				$$component = $matches[ $component ];
			}
		}

54877.patch​ looks good to me, but I think the latter suggestion is also worth exploring, as this code looks unnecessarily complicated :)

[azouamauriac]

Replying to johnjamesjacoby:

A more comprehensive change could refactor the following code, removing the foreach loop & removing the only variable-variable ($$component) in WordPress:

		foreach ( array( 'host', 'port' ) as $component ) {
			if ( ! empty( $matches[ $component ] ) ) {
				$$component = $matches[ $component ];
			}
		}

May be we can replace this foreach by

<?php
extract($matches);

??

[azouamauriac]

Replying to johnjamesjacoby:

Hi @azouamauriac 👋

absint() is a tricky thing. It’s a good idea to prevent invalid values, but it’s a bad idea to silently flip a negative user-supplied number into a positive one.

In this case, negative port numbers do not exist (and 0 is technically a “protected” port ID that no one should use) hence I went with absint() over (int) to keep MySQLi happy (preventing an error) even if an invalid (negative) port was attempted.

The ! empty() check above my code change effectively avoids a 0 value (or non-existent) port ID, but nothing checks for and avoids a negative numeric value.

I consider absint() here to be a tiny bit of defensive application protection, and I remain very open to alternative approaches and perspectives 👐

ok get it.

[SergeyBiryukov]

Replying to azouamauriac:

May be we can replace this foreach by

<?php
extract($matches);

I believe we'd want to avoid the extract() usage, as most instances were eliminated from core in #22400, except for one instance in load_template() that was left for backward compatibility. The extract() usage is also ​discouraged per the coding standards.

[azouamauriac]

ok it's ok for me. by the way I've found one usage of it in src/wp-includes/SimplePie/Enclosure.php. thank you.
Replying to SergeyBiryukov:

Replying to azouamauriac:

May be we can replace this foreach by

<?php
extract($matches);

I believe we'd want to avoid the extract() usage, as most instances were eliminated from core in #22400, except for one instance in load_template() that was left for backward compatibility. The extract() usage is also ​discouraged per the coding standards.

[johnjamesjacoby]

Replying to azouamauriac:

I've found one usage of it in src/wp-includes/SimplePie/Enclosure.php. thank you.

👍 This is in the SimplePie library, and can be reported & fixed upstream there:
​https://github.com/simplepie/simplepie/

Replying to Sergey:

the latter suggestion is also worth exploring, as this code looks unnecessarily complicated :)

💪

[jrf]

Saw mention of this ticket in Slack and thought I'd have a quick look.

If I look at the patch + look at the usage of the $port variable in various places in the wp-db.php file, I'm seeing potential problems.

The $port parameter in the mysqli::real_connect() function is optional, but by the looks of it, it is not nullable.
See: ​https://www.php.net/manual/en/mysqli.real-connect.php

As of PHP 8.1, passing null (the default value for $port in various places) to a non-nullable parameter will result in a deprecation notice and this will become a fatal error in PHP 9.0.

As the potential use of null in these calls was not yet found so far via the test suite, this indicates to me there is also a shortage of tests covering this class.

In other words, I would recommend the following actions:

1. Do a full review of the parameters passed to mysqli::real_connect()/mysqli_real_connect() function calls throughout the wp-db.php file to ensure parameter defaults passed will not cause PHP 8.1 deprecation notices.
2. Add dedicated tests to cover those situations with the risks of the deprecation notice to prevent regressions for any patches put in place.

This includes adding tests to cover the currently proposed patch.

[jrf]

@costdev did some testing regarding the non-nullablility and got confusing results, so I've now actually ​checked the PHP source and turns out the parameters ARE nullable and that the PHP manual is either wrong or just plain confusing.

Sorry for causing confusion with my previous comment, though my remark about it being a good idea to add a test to cover the current patch still stands.

[costdev]

This ticket was discussed in the bug scrub. Here are some thoughts:

• Despite touching WPDB, the patch seems safe as it eliminates invalid ports that shouldn't work anyway. - The nullable concern has been verified as an issue with PHP's documentation.
• Per the discussion in the bug scrub, adding needs-testing and needs-unit-tests.

@johnjamesjacoby do you have time to work on the unit tests in time for Beta 1 on Tuesday, April 12?

[peterwilsoncc]

Replying to jrf:

@costdev did some testing regarding the non-nullablility and got confusing results, so I've now actually ​checked the PHP source and turns out the parameters ARE nullable and that the PHP manual is either wrong or just plain confusing.

Is it possible, safe and worth defaulting to (int) ini_get( 'mysqli.default_port' ) and using that instead of null?

Following the code when there's a contradiction between the code and docs always makes me nervous. It could later be decided to be a bug and fixed with minimal warning.

[costdev]

I think this docs issue is unlikely to lead to a change in PHP source. There are open issues for other pages where nullable parameters aren't shown as such.

Having said that, here's some info about mysqli_real_connect().

• mysqli_real_connect() calls mysqli_common_connect() ​source.
• mysqli_common_connect() falls back to the default_port global when port has a falsy value. ​source.
• If the result of ini_get() is falsy, the (int) cast will change this to 0 and PHP will still fall back to the default_port global.

https://core.trac.wordpress.org/ticket/54877

@JJJ's patch just to get some tests running. --group 41722 fails due to type errors locally so seeing what happens when the full test suite runs.

[peterwilsoncc]

Replying to costdev:

I think this docs issue is unlikely to lead to a change in PHP source. There are open issues for other pages where nullable parameters aren't shown as such.

Thanks.

---

I threw JJJ's ​patch up on GitHub to get the tests running. It looks like the only tests that are failing are in Tests_DB::test_parse_db_host for a quite expected reason: type errors.

On #41722 I can't see any discussion as to why the port was passed around as a string, so I suspect an oversight.

[peterwilsoncc]

I've updated the existing tests on the database to expect the port as null|int and they are now passing on the linked pull request.

Are there additional tests needed ot be added in Tests_DB::parse_db_host_data_provider()?

[peterwilsoncc]

I think this is good to go in but how about waiting until 6.1 is forked and doing it then rather than committing it in the lead up to RC1.

[costdev]

This ticket was discussed in the recent bug scrub. Adding commit based on Peter's comment above.

@peterwilsoncc, this ticket is marked with needs-unit-tests. Can these go in during the cycle?

[chaion07]

@johnjamesjacoby thank you for reporting this. We reviewed this ticket during a recent bug-scrub. Waiting on Peter's response on Colin's question. Thanks!

[peterwilsoncc]

@costdev if additional tests are required I'd like to get them in all at once so they don't slip.

Re-reading the above, are the test related keywords above relating to the initial misunderstanding around the nullable port parameter? Given it is nullable, I can not see what additional tests are required.

[audrasjb]

@costdev are unit tests still required? If yes, it would be better to remove the commit keyword. If they are not needed for this ticket, let's remove the needs-unit-test keyword and commit.

We also need to remove the 2nd-opinion keyword if no other thoughts are needed, the ticket shouldn't have both commit and 2nd-opinion keywords at the same time :D

[costdev]

@audrasjb @peterwilsoncc I have reviewed the patch and the tests, and ran a coverage report.

TL;DR - The tests cover numeric strings and absent ports. The regex removes the need to facilitate non-numeric strings, but this doesn't appear to be covered in the datasets to protect BC. This isn't necessarily in the scope of this ticket, but could be added in this patch if desired. If not, I think the needs-unit-tests can be safely removed and the patch can go forward for commit consideration.

---

• wpdb::parse_db_host() parses using preg_match(). On successful match, the port will be a string.
• The port group of regex pattern is (?P<port>[\d]+). Non-digits should never be matched.
• $port = $port ? absint( $port ) : null; will absint() the string, returning either a positive int, or 0.
• For non-digits, $port will already be null as preg_match() won't match these. The ternary will continue to treat this as null.

The current functionality seems safe to me. However, the tests should enforce BC. The one test I can't see is one in which the port is a non-numeric string.

That is:

<?php
array(
        '127.0.0.1:port_as_string:/tmp/mysql.sock',
        false,
        '127.0.0.1',
        null, // Parsed port.
        '/tmp/mysql.sock',
        false,
),

This helps ensure that the regex is never changed to accidentally match non-digits. The coverage report screenshot below shows this dataset correctly returns $port as null.

Granted, port_as_string seems a nonsensical port value, but it fulfils the purpose of testing a non-numeric port. Any alternative non-numeric value could be used here if preferred.

[peterwilsoncc]

@costdev Thanks for the notes.

I think including the additional tests to ensure BC would be a good thing, I've pushed a few more to the ​linked pull request.

I'm removing the commit keyword as I wrote the code so need sign-off from someone else before it is commit ready.

[costdev]

Thanks @peterwilsoncc! I've reviewed and approved the PR.

[peterwilsoncc]

Thanks Colin, I'll commit.

[peterwilsoncc]

In 53670:

Database: Ensure MySQL port numbers are numeric in wpdb.

Ensure the database port number is recorded as an integer or null (the default port) when parsing the database host.

This is to prevent PHP/MySQLi throwing an exception caused by ports represented as numeric strings.

Props audrasjb, azouamauriac, chaion07, costdev, johnjamesjacoby, jrf, sergeybiryukov.
Fixes #54877.

Committed in 0a17a80bccd452b91c3b63f71f010ba131a2c954, https://core.trac.wordpress.org/changeset/53670

Thank you, everyone 🙏

[SergeyBiryukov]

In 53671:

Coding Standards: Simplify the logic for setting DB host and port in wpdb::parse_db_host().

This removes an extra foreach loop and the only variable variable ($$component) in core.

Follow-up to [20088,28342,28736-28747], [41629], [41820], [42226], [53670].

Props johnjamesjacoby.
See #54877, #55647.

[nickchomey]

Disregard this comment. I've decided to report a bug to Litespeed related to their lsphp81

[nickchomey]

#56836 was marked as a duplicate.