From c563658e3062322565f8a464e64269021cc6bee9 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Mon, 14 Oct 2024 11:33:22 -0400 Subject: [PATCH 1/7] enhance test services Allow connecting to mysqldb via socket. --- docker-compose.yaml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docker-compose.yaml b/docker-compose.yaml index 4d57753ff..2ea1d11d3 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -24,6 +24,8 @@ services: retries: 3 start_period: 20s container_name: mysqldb + volumes: + - var-run-mysqld:/var/run/mysqld redisdb: image: redis restart: always @@ -56,6 +58,7 @@ services: MYSQL_USER: admin MYSQL_PASSWD: admin MYSQL_HOST: mysqldb + MYSQL_SOCKET: /var/run/mysqld/mysqld.sock PG_HOST: postgres PG_PORT: 5432 @@ -67,6 +70,7 @@ services: volumes: - ${AGENT_CODE:-$PWD}:/usr/local/src/newrelic-php-agent + - var-run-mysqld:/var/run/mysqld entrypoint: tail command: -f /dev/null container_name: nr-php @@ -83,6 +87,7 @@ services: MYSQL_USER: admin MYSQL_PASSWD: admin MYSQL_HOST: mysqldb + MYSQL_SOCKET: /var/run/mysqld/mysqld.sock PG_HOST: postgres PG_PORT: 5432 @@ -97,8 +102,12 @@ services: NEWRELIC_LICENSE_KEY: ${NEW_RELIC_LICENSE_KEY} volumes: - ${PWD}:/usr/src/myapp + - var-run-mysqld:/var/run/mysqld working_dir: /usr/src/myapp stdin_open: true tty: true container_name: agent-devenv profiles: ["dev"] + +volumes: + var-run-mysqld: From 16b2e924c1a87c311498640d96e43ca233b7cec1 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Mon, 14 Oct 2024 11:34:53 -0400 Subject: [PATCH 2/7] enhance mysqli tests Test slowsql functionality when mysqldb is accessed via socket. --- .../mysqli/test_explain_connect_socket.php | 148 +++++++++++++++++ .../mysqli/test_explain_construct_socket.php | 150 ++++++++++++++++++ 2 files changed, 298 insertions(+) create mode 100644 tests/integration/mysqli/test_explain_connect_socket.php create mode 100644 tests/integration/mysqli/test_explain_construct_socket.php diff --git a/tests/integration/mysqli/test_explain_connect_socket.php b/tests/integration/mysqli/test_explain_connect_socket.php new file mode 100644 index 000000000..0c56e3bc9 --- /dev/null +++ b/tests/integration/mysqli/test_explain_connect_socket.php @@ -0,0 +1,148 @@ +", + "?? SQL ID", + "SELECT TABLE_NAME FROM information_schema.tables WHERE table_name=?", + "Datastore/statement/MySQL/tables/select", + 1, + "?? total time", + "?? min time", + "?? max time", + { + "explain_plan": [ + [ + "id", + "select_type", + "table", + "type", + "possible_keys", + "key", + "key_len", + "ref", + "rows", + "Extra" + ], + [ + [ + 1, + "SIMPLE", + "tables", + "ALL", + null, + "TABLE_NAME", + null, + null, + null, + "Using where; Skip_open_table; Scanned 1 database" + ] + ] + ], + "backtrace": [ + " in mysqli_stmt_execute called at __FILE__ (??)", + " in test_prepare called at __FILE__ (??)" + ] + } + ] + ] +] +*/ + +/*EXPECT_TRACED_ERRORS +null +*/ + +require_once(realpath (dirname ( __FILE__ )) . '/../../include/config.php'); + +function test_prepare($link) +{ + $query = "SELECT TABLE_NAME FROM information_schema.tables WHERE table_name='STATISTICS'"; + + $stmt = mysqli_prepare($link, $query); + if (FALSE === $stmt) { + echo mysqli_error($link) . "\n"; + return; + } + + if (FALSE === mysqli_stmt_execute($stmt)) { + echo mysqli_stmt_error($stmt) . "\n"; + return; + } + + if (FALSE === mysqli_stmt_bind_result($stmt, $value)) { + echo mysqli_stmt_error($stmt) . "\n"; + return; + } + + while (mysqli_stmt_fetch($stmt)) { + echo $value . "\n"; + } + + mysqli_stmt_close($stmt); +} + +$link = mysqli_connect('localhost', $MYSQL_USER, $MYSQL_PASSWD, $MYSQL_DB, null, $MYSQL_SOCKET); +if (mysqli_connect_errno()) { + echo mysqli_connect_error() . "\n"; + exit(1); +} + +test_prepare($link); +mysqli_close($link); diff --git a/tests/integration/mysqli/test_explain_construct_socket.php b/tests/integration/mysqli/test_explain_construct_socket.php new file mode 100644 index 000000000..20c6e1481 --- /dev/null +++ b/tests/integration/mysqli/test_explain_construct_socket.php @@ -0,0 +1,150 @@ +", + "?? SQL ID", + "SELECT TABLE_NAME FROM information_schema.tables WHERE table_name=?", + "Datastore/statement/MySQL/tables/select", + 1, + "?? total time", + "?? min time", + "?? max time", + { + "explain_plan": [ + [ + "id", + "select_type", + "table", + "type", + "possible_keys", + "key", + "key_len", + "ref", + "rows", + "Extra" + ], + [ + [ + 1, + "SIMPLE", + "tables", + "ALL", + null, + "TABLE_NAME", + null, + null, + null, + "Using where; Skip_open_table; Scanned 1 database" + ] + ] + ], + "backtrace": [ + " in mysqli_stmt_execute called at __FILE__ (??)", + " in test_prepare called at __FILE__ (??)" + ] + } + ] + ] +] +*/ + +/*EXPECT_TRACED_ERRORS +null +*/ + +require_once(realpath (dirname ( __FILE__ )) . '/../../include/config.php'); + +function test_prepare($link) +{ + + $query = "SELECT TABLE_NAME FROM information_schema.tables WHERE table_name='STATISTICS'"; + + $stmt = mysqli_prepare($link, $query); + if (FALSE === $stmt) { + echo mysqli_error($link) . "\n"; + return; + } + + if (FALSE === mysqli_stmt_execute($stmt)) { + echo mysqli_stmt_error($stmt) . "\n"; + return; + } + + if (FALSE === mysqli_stmt_bind_result($stmt, $value)) { + echo mysqli_stmt_error($stmt) . "\n"; + return; + } + + while (mysqli_stmt_fetch($stmt)) { + echo $value . "\n"; + } + + mysqli_stmt_close($stmt); +} + +$link = new mysqli('localhost', $MYSQL_USER, $MYSQL_PASSWD, $MYSQL_DB, null, $MYSQL_SOCKET); +if (mysqli_connect_errno()) { + echo mysqli_connect_error() . "\n"; + exit(1); +} + +test_prepare($link); +mysqli_close($link); From a1e0693d54003e8905f7b7b4e2a1c4d2432d16ea Mon Sep 17 00:00:00 2001 From: Konstantin Kovshenin Date: Wed, 9 Oct 2024 22:40:22 +0100 Subject: [PATCH 3/7] fix(agent): Don't skip arguments when calling mysqli::real_connect 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. This change ensures all arguments are passed on to real_connect in the correct order, including any nulls. --- agent/php_mysqli.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/agent/php_mysqli.c b/agent/php_mysqli.c index b17fd2491..0f6d66920 100644 --- a/agent/php_mysqli.c +++ b/agent/php_mysqli.c @@ -403,18 +403,22 @@ static nr_status_t nr_php_mysqli_link_real_connect( zval* retval = NULL; #define ADD_IF_INT_SET(args, argc, value) \ + args[argc] = nr_php_zval_alloc(); \ if (value) { \ - args[argc] = nr_php_zval_alloc(); \ ZVAL_LONG(args[argc], value); \ - argc++; \ - } + } else { \ + ZVAL_NULL(args[argc]); \ + } \ + argc++; #define ADD_IF_STR_SET(args, argc, value) \ + args[argc] = nr_php_zval_alloc(); \ if (value) { \ - args[argc] = nr_php_zval_alloc(); \ nr_php_zval_str(args[argc], value); \ - argc++; \ - } + } else { \ + ZVAL_NULL(args[argc]); \ + } \ + argc++; ADD_IF_STR_SET(argv, argc, nr_php_mysqli_strip_persistent_prefix(metadata->host)); From e9099acacbdcab570b17d585217c5fce4e043d06 Mon Sep 17 00:00:00 2001 From: Konstantin Kovshenin Date: Tue, 15 Oct 2024 21:40:11 +0100 Subject: [PATCH 4/7] Avoid defaulting the final $flags argument to null The correct default for the $flags argument is 0, not null. Passing a null causes a warning. --- agent/php_mysqli.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/agent/php_mysqli.c b/agent/php_mysqli.c index 0f6d66920..a0fffbd83 100644 --- a/agent/php_mysqli.c +++ b/agent/php_mysqli.c @@ -434,7 +434,13 @@ static nr_status_t nr_php_mysqli_link_real_connect( ADD_IF_STR_SET(argv, argc, metadata->database); ADD_IF_INT_SET(argv, argc, metadata->port); ADD_IF_STR_SET(argv, argc, metadata->socket); - ADD_IF_INT_SET(argv, argc, metadata->flags); + + /* + * Avoid defaulting the final $flags argument to null. + */ + if (metadata->flags) { + ADD_IF_INT_SET(argv, argc, metadata->flags); + } } retval = nr_php_call_user_func(link, "real_connect", argc, argv TSRMLS_CC); From bf7ae8b5fac15bdbcbd26342c324610f95bae375 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 15 Oct 2024 20:37:39 -0700 Subject: [PATCH 5/7] fix(agent): only non-null for first three params 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. Additionally, for our instrumentation to work, the first three parameters should only be added if they are non-null (note the comment that exists before adding the remaining params). 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 do call call mysqli::select_db. We can handle this with an additional param to specify whether a value should be added as an arg if it is set or not set. --- agent/php_mysqli.c | 61 ++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/agent/php_mysqli.c b/agent/php_mysqli.c index a0fffbd83..5ec751f9c 100644 --- a/agent/php_mysqli.c +++ b/agent/php_mysqli.c @@ -1,3 +1,5 @@ +_is_set +_set /* * Copyright 2020 New Relic Corporation. All rights reserved. * SPDX-License-Identifier: Apache-2.0 @@ -398,48 +400,53 @@ static nr_status_t nr_php_mysqli_link_real_connect( zval* link, const nr_mysqli_metadata_link_t* metadata TSRMLS_DC) { zend_ulong argc = 0; + zend_ulong arg_required = 0; zval* argv[7] = {0}; zend_ulong i; zval* retval = NULL; -#define ADD_IF_INT_SET(args, argc, value) \ - args[argc] = nr_php_zval_alloc(); \ - if (value) { \ - ZVAL_LONG(args[argc], value); \ - } else { \ - ZVAL_NULL(args[argc]); \ - } \ - argc++; - -#define ADD_IF_STR_SET(args, argc, value) \ - args[argc] = nr_php_zval_alloc(); \ - if (value) { \ - nr_php_zval_str(args[argc], value); \ - } else { \ - ZVAL_NULL(args[argc]); \ - } \ - argc++; - - ADD_IF_STR_SET(argv, argc, +#define ADD_IF_INT_SET(is_set, args, argc, value) \ + if (value) { \ + args[argc] = nr_php_zval_alloc(); \ + ZVAL_LONG(args[argc], value); \ + argc++; \ + } else if (false == is_set) { \ + args[argc] = nr_php_zval_alloc(); \ + ZVAL_NULL(args[argc]); \ + argc++; \ + } + +#define ADD_IF_STR_SET(is_set, args, argc, value) \ + if (value) { \ + args[argc] = nr_php_zval_alloc(); \ + nr_php_zval_str(args[argc], value); \ + argc++; \ + } else if (false == is_set) { \ + args[argc] = nr_php_zval_alloc(); \ + ZVAL_NULL(args[argc]); \ + argc++; \ + } + + ADD_IF_STR_SET(true, argv, argc, nr_php_mysqli_strip_persistent_prefix(metadata->host)); - ADD_IF_STR_SET(argv, argc, metadata->user); - ADD_IF_STR_SET(argv, argc, metadata->password); - + ADD_IF_STR_SET(true, argv, argc, metadata->user); + ADD_IF_STR_SET(true, argv, argc, metadata->password); /* * We can only add the remaining metadata fields if we already have three * arguments (host, user and password) above, lest we accidentally set the * wrong positional argument to something it doesn't mean. */ + arg_required = argc; if (argc == 3) { - ADD_IF_STR_SET(argv, argc, metadata->database); - ADD_IF_INT_SET(argv, argc, metadata->port); - ADD_IF_STR_SET(argv, argc, metadata->socket); + ADD_IF_STR_SET(false, argv, argc, metadata->database); + ADD_IF_INT_SET(false, argv, argc, metadata->port); + ADD_IF_STR_SET(false, argv, argc, metadata->socket); /* * Avoid defaulting the final $flags argument to null. */ if (metadata->flags) { - ADD_IF_INT_SET(argv, argc, metadata->flags); + ADD_IF_INT_SET(false, argv, argc, metadata->flags); } } @@ -460,7 +467,7 @@ static nr_status_t nr_php_mysqli_link_real_connect( * If we didn't specify the database in the connection parameters, we need to * call mysqli::select_db here. */ - if (metadata->database && (argc < 4)) { + if (metadata->database && (arg_required < 3)) { zval* database = nr_php_zval_alloc(); nr_php_zval_str(database, metadata->database); From 312228d28940d52026411dc528d878bf5b9dbbef Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Tue, 15 Oct 2024 20:39:00 -0700 Subject: [PATCH 6/7] fix(agent): typo --- agent/php_mysqli.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/agent/php_mysqli.c b/agent/php_mysqli.c index 5ec751f9c..4c7192d6a 100644 --- a/agent/php_mysqli.c +++ b/agent/php_mysqli.c @@ -1,5 +1,3 @@ -_is_set -_set /* * Copyright 2020 New Relic Corporation. All rights reserved. * SPDX-License-Identifier: Apache-2.0 From b1c7ab503d97f9c0b67dba9d5a92ed36a8c35902 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Wed, 16 Oct 2024 09:46:26 -0700 Subject: [PATCH 7/7] fix(agent): variable naming, remove redundant check. * changed is_set to null_ok * removed extra check on flags since we can specify null_ok to false. --- agent/php_mysqli.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/agent/php_mysqli.c b/agent/php_mysqli.c index 4c7192d6a..24a321b02 100644 --- a/agent/php_mysqli.c +++ b/agent/php_mysqli.c @@ -403,52 +403,46 @@ static nr_status_t nr_php_mysqli_link_real_connect( zend_ulong i; zval* retval = NULL; -#define ADD_IF_INT_SET(is_set, args, argc, value) \ +#define ADD_IF_INT_SET(null_ok, args, argc, value) \ if (value) { \ args[argc] = nr_php_zval_alloc(); \ ZVAL_LONG(args[argc], value); \ argc++; \ - } else if (false == is_set) { \ + } else if (true == null_ok) { \ args[argc] = nr_php_zval_alloc(); \ ZVAL_NULL(args[argc]); \ argc++; \ } -#define ADD_IF_STR_SET(is_set, args, argc, value) \ +#define ADD_IF_STR_SET(null_ok, args, argc, value) \ if (value) { \ args[argc] = nr_php_zval_alloc(); \ nr_php_zval_str(args[argc], value); \ argc++; \ - } else if (false == is_set) { \ + } else if (true == null_ok) { \ args[argc] = nr_php_zval_alloc(); \ ZVAL_NULL(args[argc]); \ argc++; \ } - ADD_IF_STR_SET(true, argv, argc, + ADD_IF_STR_SET(false, argv, argc, nr_php_mysqli_strip_persistent_prefix(metadata->host)); - ADD_IF_STR_SET(true, argv, argc, metadata->user); - ADD_IF_STR_SET(true, argv, argc, metadata->password); + ADD_IF_STR_SET(false, argv, argc, metadata->user); + ADD_IF_STR_SET(false, argv, argc, metadata->password); + /* * We can only add the remaining metadata fields if we already have three * arguments (host, user and password) above, lest we accidentally set the - * wrong positional argument to something it doesn't mean. + * wrong positional argument to something it doesn't mean. Note, prior + * to 7.4 not all args are nullable. */ arg_required = argc; if (argc == 3) { - ADD_IF_STR_SET(false, argv, argc, metadata->database); - ADD_IF_INT_SET(false, argv, argc, metadata->port); - ADD_IF_STR_SET(false, argv, argc, metadata->socket); - - /* - * Avoid defaulting the final $flags argument to null. - */ - if (metadata->flags) { - ADD_IF_INT_SET(false, argv, argc, metadata->flags); - } - } - - retval = nr_php_call_user_func(link, "real_connect", argc, argv TSRMLS_CC); + ADD_IF_STR_SET(true, argv, argc, metadata->database); + ADD_IF_INT_SET(true, argv, argc, metadata->port); + ADD_IF_STR_SET(true, argv, argc, metadata->socket); + ADD_IF_INT_SET(false, argv, argc, metadata->flags); + } retval = nr_php_call_user_func(link, "real_connect", argc, argv TSRMLS_CC); for (i = 0; i < argc; i++) { nr_php_zval_free(&argv[i]);