fix(agent): Don't skip arguments when calling mysqli::real_connect

[kovshenin]

When one of the arguments in nr_php_mysqli_link_real_connect is null, the entire argument is skipped. This causes a fatal error when attempting to connect to a database through a Unix socket, without specifying a port number.

Here are two sample connection strings:

$mysqli = new mysqli( 'mysqldb', 'admin', 'admin', 'database', 3306 );
$mysqli = new mysqli( 'localhost', 'admin', 'admin', 'database', null, '/usr/src/myapp/sockets/mysql.sock' );

If you run a slow query against these, one that would trigger the additional EXPLAIN routine, the agent will fall into the nr_php_mysqli_link_duplicate routine, and eventually into nr_php_mysqli_link_real_connect where it calls mysqli::real_connect. The first link's arguments will be accurate:

0: (string) mysqldb
1: (string) admin
2: (string) admin
3: (string) database
4: (int) 3306

However, the second link will choke on the (lack of) port:

0: (string) localhost
1: (string) admin
2: (string) admin
3: (string) database
4: (string) /usr/src/myapp/sockets/mysql.sock

The mysqli::real_connect method expects the fifth parameter to be an integer port number, but is going to get a string instead. With recent versions of PHP this fails with a fatal error:

Fatal error: Uncaught TypeError: mysqli::real_connect(): Argument #5 ($port) must be of type ?int, string given

The proposed change ensures all arguments are passed on to real_connect in the correct order, including any nulls.

Somewhat related to #881 I guess.

[CLAassistant]

All committers have signed the CLA.

[lavarou]

@kovshenin Thank you for engaging and your interest in improving New Relic’s PHP instrumentation! Could you update this PR and set dev as the base branch? Here’s a picture how to do it after you click on the ‘Edit’ button next to PR’s title:

[kovshenin]

@lavarou changed and rebased.

[lavarou]

@kovshenin Thank you for your contribution! We looked at the suggested changes and enhanced our tests to test it out - see #976. This PR indeed addresses the issue from the description, and the tests added in #976 pass. However it caused a few existing tests to fail:

• PHPs 7.2, 7.3 and 7.4:
  • tests/integration/mysqli/test_explain_database_no_user.php
  • tests/integration/mysqli/test_explain_database_no_user_logging_off.php
• PHPs 8.0, 8.1, 8.2 and 8.3:
  • tests/integration/mysqli/test_explain_reused_id.php

These test failures will need us to further investigate and allocate time to resolve. If you wish to investigate the failures locally, you can use the containerized development environment to build the agent and run the tests locally (only the New Relic license key is required).

[kovshenin]

Thanks for taking a look @lavarou!

I totally missed the fact that the $flags argument defaults to 0 and not null. I believe I've addressed this in e937403. However I am seeing some other test failures around jit and some remote requests (X-Newrelic-App-Data, X-NewRelic-ID headers) which I don't think are related, or perhaps my license key is misconfigured.

[kovshenin]

Hmm, I see some of the explain tests failed again. I'm having a hard time reproducing these in my environment. Here's what I'm running from within a dev-shell:

INTEGRATION_ARGS='-pattern="test_explain*" -agent ./agent/.libs/newrelic.so' make integration-tests

And I get:

pass - tests/integration/mysqli/test_explain_database_no_user.php
pass - tests/integration/mysqli/test_explain_database_no_user_logging_off.php

In all PHP versions. I'd appreciate some clues as I am very new to this :) Thanks!

[zsistla]

Hmm, I see some of the explain tests failed again. I'm having a hard time reproducing these in my environment. Here's what I'm running from within a dev-shell:

INTEGRATION_ARGS='-pattern="test_explain*" -agent ./agent/.libs/newrelic.so' make integration-tests

And I get:

pass - tests/integration/mysqli/test_explain_database_no_user.php
pass - tests/integration/mysqli/test_explain_database_no_user_logging_off.php

In all PHP versions. I'd appreciate some clues as I am very new to this :) Thanks!

Hi @kovshenin
Thanks again for PR!

It looks like part of the problem is that argc in the function previously only incremented for non-null parameters, so it's throwing the count off when we are using it in if statements. argc was basically being used for both parameter count and to track non-null settings.
Additionally, for our instrumentation to work with PHP < 7.4, the first three parameters should only be added if they are non-null (note the comment that exists before adding the remaining params here). So we need to keep track of the required first three in addition to all other arguments that are being added. We also need the count of the required parameters so that we can properly assess whether we need to call call mysqli::select_db here. We can handle this with an additional param to specify whether it should be added if it is set or not set. This also accommodates PHP < 7.4 before which all parameters for real_connect were not nullable.

Please check out the changes added #976 to and see if you agree. These change passed all tests.

[kovshenin]

Thanks for the explanation @zsistla, that makes a lot of sense, I thinks this will work! I left a couple of comments on the PR.

[lavarou]

Hmm, I see some of the explain tests failed again. I'm having a hard time reproducing these in my environment. Here's what I'm running from within a dev-shell:

INTEGRATION_ARGS='-pattern="test_explain*" -agent ./agent/.libs/newrelic.so' make integration-tests

And I get:

pass - tests/integration/mysqli/test_explain_database_no_user.php
pass - tests/integration/mysqli/test_explain_database_no_user_logging_off.php

In all PHP versions. I'd appreciate some clues as I am very new to this :) Thanks!

@kovshenin How do you start the dev-shell for different PHPs? What is the output of php --version in dev-shell once you start it? I ask this because I think the user-friendliness of the containerized development environment can be improved - see #977.

[kovshenin]

@lavarou I just run make dev-shell which gives me PHP 8.3. However when I run the integration tests from within the dev container, I'm assuming it somehow magically switches the PHP version from NRLAMP_PHP, I haven't looked into what the runner actually does.

Did I run all my tests under 8.3 only? Does make dev-integration-tests also run under a single PHP version? I probably misunderstood how it is supposed to work.

[ZNeumann]

Closing this PR - it was incorporated into #976 and released in 11.3. Thanks again for your contribution!

[lavarou]

@lavarou I just run make dev-shell which gives me PHP 8.3. However when I run the integration tests from within the dev container, I'm assuming it somehow magically switches the PHP version from NRLAMP_PHP, I haven't looked into what the runner actually does.

Hi @kovshenin! I assume you must have used make integration or make integration-tests command from within devenv service container. Unfortunately these are not meant to be used from within devenv service container, because it only has a single PHP available (default is 8.3). These targets were used by legacy build system, which used NRLAPMP_PHP to switch PHPs and newrelic.so used by integration_runner.

Did I run all my tests under 8.3 only?

Yes.

Does make dev-integration-tests also run under a single PHP version?

Yes.

I probably misunderstood how it is supposed to work.

No worries! We appreciate your attempt to use containerized development environment and this feedback is very valuable! We'll try to make containerized development environment more user friendly - see #977.

[kovshenin]

Hi @lavarou yeah it makes sense now, also explains why my tests weren't failing 🤣 I was actually pleasantly surprised with the containerized dev environment, it felt very neat and I had most things working without having to jump through hoops with a million dependencies on my local system. Having the ability to set a PHP version for the shell would be a great addition to that, thanks for working on it!