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

Some transaction globals were leaked when calling
`newrelic_end_transaction(true)`. The freeing of these transaction
globals has been moved to join all the other transaction globals to
alleviate this.

In so doing, additional rshutdown methods are needed. This is because
the transaction globals contain reference incremented zvals. If we wait
until postdeactivate, those zvals will be in an undefined state and the
data structures containing those references cannot be properly freed. In
line with other transaction globals, we trigger a shutdown method during
rshutdown to free these globals while the zvals are still valid.

Author: ZNeumann

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

@@ -1191,9 +1191,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) {
@@ -1360,6 +1357,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();