fix(agent): fix potential memleak when calling newrelic_end_transaction(true)

agent/php_mysqli.c

@@ -688,3 +688,28 @@ const char* nr_php_mysqli_strip_persistent_prefix(const char* host) {
 
   return host;
 }
+
+void nr_php_mysqli_rshutdown() {
+  /*
+   * This frees mysqli metadata stored in the transaction.
+   *
+   * `mysqli_queries` contains duplicates of zvals. If
+   * `nr_php_txn_end` is called from the post-deactivate callback, request
+   * shutdown functions have already been called; and the Zend VM has already
+   * forcefully freed all dangling zvals that are not referenced by the global
+   * scope (regardless of their reference count), thus leaving the zvals stored
+   * in the mysqli_queries metadata in an "undefined" state. Consequently,
+   * freeing the zvals in `nr_php_txn_end` at this stage can result in undefined
+   * behavior.
+   *
+   * Calling this function during the RSHUTDOWN phase ensures that the zvals in
+   * `mysqli_queries` are cleaned up before Zend winds down the VM and
+   * forcefully frees zvals.
+   *
+   * If `nr_php_txn_end` is called outside the post-deactivate callback,
+   * it frees `mysqli_queries` by itself.
+   */
+  if (nrlikely(NRPRG(txn))) {
+    nr_hashmap_destroy(&NRTXNGLOBAL(mysqli_queries));
+  }
+}

agent/php_mysqli.h

@@ -163,4 +163,10 @@ extern nr_datastore_instance_t* nr_php_mysqli_retrieve_datastore_instance(
 extern void nr_php_mysqli_remove_datastore_instance(
     const zval* mysqli_obj TSRMLS_DC);
 
+/*
+ * Purpose : Frees reference incremented, transaction global zvals
+ *           that must be cleaned up prior to postdeactivate
+ */
+extern void nr_php_mysqli_rshutdown();
+
 #endif /* PHP_MYSQLI_HDR */

agent/php_pdo.c

@@ -638,3 +638,28 @@ zval* nr_php_pdo_disable_persistence(const zval* options TSRMLS_DC) {
   nr_php_zval_free(&persistent);
   return result;
 }
+
+void nr_php_pdo_rshutdown() {
+  /*
+   * This frees pdo metadata stored in the transaction.
+   *
+   * `pdo_link_options` contains duplicates of zvals. If
+   * `nr_php_txn_end` is called from the post-deactivate callback, request
+   * shutdown functions have already been called; and the Zend VM has already
+   * forcefully freed all dangling zvals that are not referenced by the global
+   * scope (regardless of their reference count), thus leaving the zvals stored
+   * in the pdo_link_options metadata in an "undefined" state. Consequently,
+   * freeing the zvals in `nr_php_txn_end` at this stage can result in undefined
+   * behavior.
+   *
+   * Calling this function during the RSHUTDOWN phase ensures that the zvals in
+   * `pdo_link_options` are cleaned up before Zend winds down the VM and
+   * forcefully frees zvals.
+   *
+   * If `nr_php_txn_end` is called outside the post-deactivate callback,
+   * it frees `pdo_link_options` by itself.
+   */
+  if (nrlikely(NRPRG(txn))) {
+    nr_hashmap_destroy(&NRTXNGLOBAL(pdo_link_options));
+  }
+}

agent/php_pdo.h

@@ -161,4 +161,9 @@ extern nr_status_t nr_php_pdo_parse_data_source(
 extern void nr_php_pdo_free_data_sources(struct pdo_data_src_parser* parsed,
                                          size_t nparams);
 
+/*
+ * Purpose : Frees reference incremented, transaction global zvals
+ *           that must be cleaned up prior to postdeactivate
+ */
+extern void nr_php_pdo_rshutdown();
 #endif

agent/php_rshutdown.c

@@ -12,6 +12,8 @@
 #include "php_globals.h"
 #include "php_user_instrument.h"
 #include "php_wrapper.h"
+#include "php_mysqli.h"
+#include "php_pdo.h"
 #include "util_logging.h"
 #include "lib_guzzle4.h"
 
@@ -49,6 +51,8 @@ PHP_RSHUTDOWN_FUNCTION(newrelic) {
 
   nr_guzzle4_rshutdown(TSRMLS_C);
   nr_curl_rshutdown(TSRMLS_C);
+  nr_php_pdo_rshutdown();
+  nr_php_mysqli_rshutdown();
 
   nrl_verbosedebug(NRL_INIT, "RSHUTDOWN processing done");
 

agent/php_txn.c

@@ -1187,9 +1187,6 @@ static void nr_php_txn_do_shutdown(nrtxn_t* txn TSRMLS_DC) {
    * cannot be configured into the browser client config.
    */
   nr_php_capture_request_parameters(txn TSRMLS_CC);
-
-  nr_hashmap_destroy(&NRTXNGLOBAL(mysqli_queries));
-  nr_hashmap_destroy(&NRTXNGLOBAL(pdo_link_options));
 }
 
 void nr_php_txn_shutdown(TSRMLS_D) {
@@ -1356,6 +1353,9 @@ nr_status_t nr_php_txn_end(int ignoretxn, int in_post_deactivate TSRMLS_DC) {
   nr_hashmap_destroy(&NRTXNGLOBAL(curl_multi_metadata));
 
   nr_mysqli_metadata_destroy(&NRTXNGLOBAL(mysqli_links));
+  nr_hashmap_destroy(&NRTXNGLOBAL(mysqli_queries));
+
+  nr_hashmap_destroy(&NRTXNGLOBAL(pdo_link_options));
 
   return NR_SUCCESS;
 }

tests/integration/external/curl_multi_exec/test_txn_restart_ignore.php

@@ -0,0 +1,96 @@
+<?php
+/*
+ * Copyright 2020 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/*DESCRIPTION
+Tests that transaction globals are properly freed when using New Relic API
+*/
+
+/*SKIPIF
+<?php
+if (!extension_loaded("curl")) {
+  die("skip: curl extension required");
+}
+*/
+
+/*INI
+newrelic.transaction_tracer.threshold=0
+newrelic.cross_application_tracer.enabled = false
+newrelic.distributed_tracing_enabled=0
+*/
+
+/*EXPECT
+X-NewRelic-ID=missing X-NewRelic-Transaction=missing tracing endpoint reached
+X-NewRelic-ID=missing X-NewRelic-Transaction=missing tracing endpoint reached
+ok - end of function reached without crash
+*/
+
+
+/* the following metrics would be expected as well but due to an issue on the creation 
+ * of these metrics when a transaction is stopped and new one started they do not
+ * currently show up.
+ * [{"name": "Supportability/Logging/Forwarding/PHP/enabled"},                   [1, "??", "??", "??", "??", "??"]],
+ * [{"name": "Supportability/Logging/Metrics/PHP/enabled"},                      [1, "??", "??", "??", "??", "??"]]
+*/
+
+/*EXPECT_METRICS
+[
+  "?? agent run id",
+  "?? start time",
+  "?? stop time",
+  [
+    [{"name":"External/all"},                                [1, "??", "??", "??", "??", "??"]],
+    [{"name":"External/allOther"},                           [1, "??", "??", "??", "??", "??"]],
+    [{"name":"External/127.0.0.1/all"},                      [1, "??", "??", "??", "??", "??"]],
+    [{"name":"External/127.0.0.1/all",
+      "scope":"OtherTransaction/php__FILE__"},               [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransaction/all"},                        [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransaction/php__FILE__"},                [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransactionTotalTime"},                   [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransactionTotalTime/php__FILE__"},       [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/api/start_transaction"},        [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Metrics/PHP/enabled"},  [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Labels/PHP/disabled"},  [1, "??", "??", "??", "??", "??"]]
+  ]
+]
+*/
+
+
+
+
+require_once(realpath(dirname(__FILE__)) . '/../../../include/tap.php');
+require_once(realpath(dirname(__FILE__)) . '/../../../include/config.php');
+
+function test_txn_restart() {
+  $url = make_tracing_url(realpath(dirname(__FILE__)) . '/../../../include/tracing_endpoint.php');
+
+  $ch1 = curl_init($url);
+  $ch2 = curl_init($url);
+  $mh = curl_multi_init();
+
+  $active = 0;
+
+  curl_multi_add_handle($mh, $ch1);
+  curl_multi_exec($mh, $active);
+
+  newrelic_ignore_transaction();
+  newrelic_end_transaction();
+  newrelic_start_transaction(ini_get("newrelic.appname"));
+
+  curl_multi_add_handle($mh, $ch2);
+  do {
+    curl_multi_exec($mh, $active);
+  } while ($active > 0);
+
+  curl_close($ch1);
+  curl_close($ch2);
+  curl_multi_close($mh);
+
+  tap_ok("end of function reached without crash", true);
+}
+
+test_txn_restart();

tests/integration/external/guzzle7/test_txn_ignore.php

@@ -0,0 +1,98 @@
+<?php
+/*
+ * Copyright 2020 New Relic Corporation. All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/*DESCRIPTION
+Test that transaction globals are properly freed when using New Relic API
+ */
+
+/*SKIPIF
+<?php
+require("skipif.inc");
+*/
+
+/*INI
+*/
+
+/*EXPECT
+traceparent=found tracestate=found newrelic=found X-NewRelic-ID=missing X-NewRelic-Transaction=missing tracing endpoint reached
+traceparent=found tracestate=found newrelic=found X-NewRelic-ID=missing X-NewRelic-Transaction=missing tracing endpoint reached
+traceparent=found tracestate=found newrelic=found X-NewRelic-ID=missing X-NewRelic-Transaction=missing Customer-Header=found tracing endpoint reached
+traceparent=found tracestate=found newrelic=found X-NewRelic-ID=missing X-NewRelic-Transaction=missing tracing endpoint reached
+traceparent=found tracestate=found newrelic=found X-NewRelic-ID=missing X-NewRelic-Transaction=missing tracing endpoint reached
+traceparent=found tracestate=found newrelic=found X-NewRelic-ID=missing X-NewRelic-Transaction=missing Customer-Header=found tracing endpoint reached
+*/
+
+/*EXPECT_RESPONSE_HEADERS
+*/
+
+/*EXPECT_METRICS
+[
+  "?? agent run id",
+  "?? timeframe start",
+  "?? timeframe stop",
+  [
+    [{"name":"External/127.0.0.1/all"},                   [3, "??", "??", "??", "??", "??"]],
+    [{"name":"External/127.0.0.1/all",
+      "scope":"OtherTransaction/php__FILE__"},            [3, "??", "??", "??", "??", "??"]],
+    [{"name":"External/all"},                             [3, "??", "??", "??", "??", "??"]],
+    [{"name":"External/allOther"},                        [3, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransaction/all"},                     [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransaction/php__FILE__"},             [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransactionTotalTime"},                [1, "??", "??", "??", "??", "??"]],
+    [{"name":"OtherTransactionTotalTime/php__FILE__"},    [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/api/start_transaction"},     [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/api/end_transaction"},       [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/PHP/package/guzzlehttp/guzzle/7/detected"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Unsupported/curl_setopt/CURLOPT_HEADERFUNCTION/closure"}, [3, 0, 0, 0, 0, 0]],
+    [{"name":"DurationByCaller/Unknown/Unknown/Unknown/Unknown/all"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"DurationByCaller/Unknown/Unknown/Unknown/Unknown/allOther"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/TraceContext/Create/Success"}, [3, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/DistributedTrace/CreatePayload/Success"}, [3, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]],
+    [{"name":"Supportability/Logging/Labels/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]]
+  ]
+]
+*/
+
+
+?>
+<?php
+require_once(realpath(dirname(__FILE__)) . '/../../../include/config.php');
+require_once(realpath(dirname(__FILE__)) . '/../../../include/unpack_guzzle.php');
+require_guzzle(7);
+
+use GuzzleHttp\Client;
+
+function run_test() {
+    /* Create URL. */
+    $url = "http://" . make_tracing_url(realpath(dirname(__FILE__)) .  '/../../../include/tracing_endpoint.php');
+
+    $client = new Client();
+    $response = $client->get($url);
+    echo $response->getBody();
+
+    $response = $client->get($url, [
+        'headers' => [
+            'zip' => 'zap']]);
+    echo $response->getBody();
+
+    $response = $client->get($url, [
+        'headers' => [
+            'zip' => 'zap',
+            CUSTOMER_HEADER => 'zap']]);
+    echo $response->getBody();
+}
+
+run_test();
+
+newrelic_end_transaction(true);
+newrelic_start_transaction(ini_get("newrelic.appname"));
+
+run_test();
+
+newrelic_end_transaction();