fix(agent): don't skip arguments when calling mysqli::real_connect (#976)

lavarou · kovshenin · zsistla · web-flow · commit dc162a4eb72f · 2024-10-18T07:30:16.000-07:00
Co-authored-by: Konstantin Kovshenin &lt;kovshenin@gmail.com&gt;
Co-authored-by: Amber Sistla &lt;asistla@newrelic.com&gt;
diff --git a/agent/php_mysqli.c b/agent/php_mysqli.c
@@ -398,42 +398,51 @@ 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) \
-  if (value) {                            \
-    args[argc] = nr_php_zval_alloc();     \
-    ZVAL_LONG(args[argc], value);         \
-    argc++;                               \
-  }
-
-#define ADD_IF_STR_SET(args, argc, value) \
-  if (value) {                            \
-    args[argc] = nr_php_zval_alloc();     \
-    nr_php_zval_str(args[argc], value);   \
-    argc++;                               \
-  }
-
-  ADD_IF_STR_SET(argv, argc,
+#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 (true == null_ok) {                   \
+    args[argc] = nr_php_zval_alloc();             \
+    ZVAL_NULL(args[argc]);                        \
+    argc++;                                       \
+  }
+
+#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 (true == null_ok) {                   \
+    args[argc] = nr_php_zval_alloc();             \
+    ZVAL_NULL(args[argc]);                        \
+    argc++;                                       \
+  }
+
+  ADD_IF_STR_SET(false, 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(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(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);
-  }
-
-  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]);
@@ -450,7 +459,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);
diff --git 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:
diff --git a/tests/integration/mysqli/test_explain_connect_socket.php b/tests/integration/mysqli/test_explain_connect_socket.php
@@ -0,0 +1,148 @@
+<?php
+/*
+ * Copyright 2020 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/*DESCRIPTION
+The agent should generate explain plans when connections are made with
+mysqli_connect() when connecting to the database via socket.
+*/
+
+/*SKIPIF
+<?php require("skipif.inc");
+*/
+
+/*INI
+error_reporting = E_ALL & ~E_DEPRECATED
+newrelic.transaction_tracer.explain_enabled = true
+newrelic.transaction_tracer.explain_threshold = 0
+newrelic.transaction_tracer.record_sql = obfuscated
+*/
+
+/*EXPECT
+STATISTICS
+*/
+
+/*EXPECT_METRICS
+[
+  "?? agent run id",
+  "?? start time",
+  "?? stop time",
+  [
+    [{"name":"DurationByCaller/Unknown/Unknown/Unknown/Unknown/all"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"DurationByCaller/Unknown/Unknown/Unknown/Unknown/allOther"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Datastore/all"},                                        [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Datastore/allOther"},                                   [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Datastore/MySQL/all"},                                  [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Datastore/MySQL/allOther"},                             [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Datastore/statement/MySQL/tables/select"},              [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Datastore/statement/MySQL/tables/select",
+      "scope":"OtherTransaction/php__FILE__"},                        [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Datastore/operation/MySQL/select"},                     [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransaction/all"},                                 [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransaction/php__FILE__"},                         [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransactionTotalTime"},                            [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransactionTotalTime/php__FILE__"},                [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Forwarding/PHP/enabled"},        [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Metrics/PHP/enabled"},           [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"},  [1, "??", "??", "??", "??", "??"]]
+  ]
+]
+*/
+
+
+
+/*EXPECT_SLOW_SQLS
+[
+  [
+    [
+      "OtherTransaction/php__FILE__",
+      "<unknown>",
+      "?? 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
