Make WordPress Core

Opened 2 years ago

Closed 20 months ago

Last modified 17 months ago

#54877 closed defect (bug) (fixed)

Occasional PHP exception being thrown on WPDB/MySQLi connections

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version: 1.5
Component: Database Keywords: has-patch needs-testing early commit
Focuses: Cc:

Description (last modified by 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

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

CodeIgniter example & fix

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

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

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

PHP MySQLi Docs

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;

Attachments (2)

54877.patch (295 bytes) - added by johnjamesjacoby 2 years ago.
54877-non-numeric-coverage.jpg (97.5 KB) - added by costdev 20 months ago.

Download all attachments as: .zip

Change History (45)

#1 @johnjamesjacoby
2 years ago

  • Description modified (diff)

#2 @johnjamesjacoby
2 years ago

  • Description modified (diff)

#3 follow-ups: @johnjamesjacoby
2 years ago

  • Keywords has-patch added

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 ];
			}
		}

This ticket was mentioned in Slack in #core by jjj. View the logs.


2 years ago

#5 @azouamauriac
2 years ago

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?

#6 follow-up: @johnjamesjacoby
2 years ago

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 👐

#7 in reply to: ↑ 3 @SergeyBiryukov
2 years ago

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 :)

#8 in reply to: ↑ 3 ; follow-up: @azouamauriac
2 years ago

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);

??

#9 in reply to: ↑ 6 @azouamauriac
2 years ago

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.

#10 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
2 years ago

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.

#11 in reply to: ↑ 10 ; follow-up: @azouamauriac
2 years ago

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.

#12 in reply to: ↑ 11 @johnjamesjacoby
2 years ago

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 :)

💪

This ticket was mentioned in Slack in #core by costdev. View the logs.


23 months ago

#14 @jrf
23 months ago

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.

#15 follow-up: @jrf
23 months ago

@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.

#16 @costdev
23 months ago

  • Keywords needs-testing needs-unit-tests added

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?

#17 in reply to: ↑ 15 @peterwilsoncc
23 months ago

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.

#18 follow-up: @costdev
23 months ago

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.
Last edited 23 months ago by costdev (previous) (diff)

#19 @costdev
23 months ago

  • Type changed from enhancement to defect (bug)

This ticket was mentioned in PR #2583 on WordPress/wordpress-develop by peterwilsoncc.


23 months ago
#20

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.

#21 in reply to: ↑ 18 @peterwilsoncc
23 months ago

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.

#22 @peterwilsoncc
23 months ago

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()?

#23 @peterwilsoncc
22 months ago

  • Keywords early added
  • Milestone changed from 6.0 to 6.1

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.

This ticket was mentioned in Slack in #core by costdev. View the logs.


21 months ago

#25 @costdev
21 months ago

  • Keywords commit added

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?

This ticket was mentioned in Slack in #core by chaion07. View the logs.


21 months ago

#27 @chaion07
21 months ago

@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!

#28 @peterwilsoncc
21 months ago

@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.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


21 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


20 months ago

#32 @audrasjb
20 months ago

@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

#33 @costdev
20 months ago

@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.

#34 @peterwilsoncc
20 months ago

  • Keywords needs-unit-tests commit removed

@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.

#35 @costdev
20 months ago

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

#36 @peterwilsoncc
20 months ago

  • Keywords commit added; 2nd-opinion removed
  • Owner set to peterwilsoncc
  • Status changed from new to assigned

Thanks Colin, I'll commit.

#37 @peterwilsoncc
20 months ago

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

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.

peterwilsoncc commented on PR #2583:


20 months ago
#38

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

JJJ commented on PR #2583:


20 months ago
#39

Thank you, everyone 🙏

#40 @SergeyBiryukov
20 months ago

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.

This ticket was mentioned in Slack in #hosting-community by javier. View the logs.


20 months ago

#42 @nickchomey
17 months ago

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

Last edited 17 months ago by nickchomey (previous) (diff)

#43 @nickchomey
17 months ago

#56836 was marked as a duplicate.

Note: See TracTickets for help on using tickets.