From a53c360f5056dec2815f90622ce48c9f80931f79 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Mon, 4 Dec 2023 16:30:00 -0500 Subject: [PATCH] refactor: suggestion to #765 (#789) A couple of suggestions to #765: 1. Save time by using pre-calculated string lengths vs calling strlen whenever possible 2. Move `nr_file_basename` as `nr_wordpress_strip_php_suffix` back to `fw_wordpress.c` --- agent/fw_wordpress.c | 51 ++++++++++++++++++------- agent/php_agent.h | 5 +++ axiom/tests/test_matcher.c | 40 ++++++++++++++++++++ axiom/util_matcher.c | 77 ++++++++++++++++++++++++++------------ axiom/util_matcher.h | 2 + axiom/util_strings.h | 32 ---------------- 6 files changed, 139 insertions(+), 68 deletions(-) diff --git a/agent/fw_wordpress.c b/agent/fw_wordpress.c index ed886204f..425330f1d 100644 --- a/agent/fw_wordpress.c +++ b/agent/fw_wordpress.c @@ -53,6 +53,27 @@ static nr_matcher_t* create_matcher_for_constant(const char* constant, return NULL; } +/* + * Purpose : Strip the ".php" file extension from a file name + * + * Params : 1. The string filename + * 2. The filename length + * + * Returns : A newly allocated string stripped of the .php extension + * + */ +static inline char* nr_wordpress_strip_php_suffix(char* filename, int filename_len) { + char* retval = NULL; + + if (!nr_striendswith(filename, filename_len, NR_PSTR(".php"))) { + return filename; + } + + retval = nr_strndup(filename, filename_len - (sizeof(".php") - 1)); + nr_free(filename); + return retval; +} + static nr_matcher_t* nr_wordpress_core_matcher() { nr_matcher_t* matcher = NULL; @@ -149,24 +170,27 @@ static nr_matcher_t* nr_wordpress_theme_matcher() { char* nr_php_wordpress_plugin_match_matcher(const char* filename) { char* plugin = NULL; - plugin = nr_matcher_match(nr_wordpress_plugin_matcher(), filename); - plugin = nr_file_basename(plugin, nr_strlen(plugin)); + int plugin_len; + plugin = nr_matcher_match_ex(nr_wordpress_plugin_matcher(), filename, nr_strlen(filename), &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); nr_matcher_destroy(&NRPRG(wordpress_plugin_matcher)); return plugin; } char* nr_php_wordpress_theme_match_matcher(const char* filename) { char* theme = NULL; - theme = nr_matcher_match(nr_wordpress_theme_matcher(), filename); - theme = nr_file_basename(theme, nr_strlen(theme)); + int plugin_len; + theme = nr_matcher_match_ex(nr_wordpress_theme_matcher(), filename, nr_strlen(filename), &plugin_len); + theme = nr_wordpress_strip_php_suffix(theme, plugin_len); nr_matcher_destroy(&NRPRG(wordpress_theme_matcher)); return theme; } char* nr_php_wordpress_core_match_matcher(const char* filename) { char* core = NULL; - core = nr_matcher_match_core(nr_wordpress_core_matcher(), filename); - core = nr_file_basename(core, nr_strlen(core)); + int plugin_len; + core = nr_matcher_match_core_ex(nr_wordpress_core_matcher(), filename, nr_strlen(filename), &plugin_len); + core = nr_wordpress_strip_php_suffix(core, plugin_len); nr_matcher_destroy(&NRPRG(wordpress_core_matcher)); return core; } @@ -193,6 +217,7 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { const char* filename = NULL; size_t filename_len; char* plugin = NULL; + int plugin_len; if (NULL == func) { return NULL; @@ -206,7 +231,7 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { NRP_PHP(NRPRG(wordpress_tag))); return NULL; } - filename_len = nr_strlen(filename); + filename_len = nr_php_function_filename_len(func); if (NRPRG(wordpress_file_metadata)) { if (nr_hashmap_get_into(NRPRG(wordpress_file_metadata), filename, @@ -224,20 +249,20 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { "Wordpress: NOT found in cache: " "filename=" NRP_FMT, NRP_FILENAME(filename)); - plugin = nr_matcher_match(nr_wordpress_plugin_matcher(), filename); - plugin = nr_file_basename(plugin, nr_strlen(plugin)); + plugin = nr_matcher_match_ex(nr_wordpress_plugin_matcher(), filename, filename_len, &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); if (plugin) { goto cache_and_return; } - plugin = nr_matcher_match(nr_wordpress_theme_matcher(), filename); - plugin = nr_file_basename(plugin, nr_strlen(plugin)); + plugin = nr_matcher_match_ex(nr_wordpress_theme_matcher(), filename, filename_len, &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); if (plugin) { goto cache_and_return; } - plugin = nr_matcher_match_core(nr_wordpress_core_matcher(), filename); - plugin = nr_file_basename(plugin, nr_strlen(plugin)); + plugin = nr_matcher_match_core_ex(nr_wordpress_core_matcher(), filename, filename_len, &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); if (plugin) { /* * The core wordpress functions are anonymized, so we don't need to return diff --git a/agent/php_agent.h b/agent/php_agent.h index 7877325d3..1c024dc54 100644 --- a/agent/php_agent.h +++ b/agent/php_agent.h @@ -702,6 +702,11 @@ nr_php_op_array_scope_name(const zend_op_array* op_array) { return NULL; } +static inline nr_string_len_t NRPURE +nr_php_function_filename_len(zend_function* func) { + return func ? nr_php_op_array_file_name_len(&func->op_array) : 0; +} + static inline const char* NRPURE nr_php_ini_entry_name(const zend_ini_entry* entry) { #if ZEND_MODULE_API_NO >= ZEND_7_0_X_API_NO /* PHP 7.0+ */ diff --git a/axiom/tests/test_matcher.c b/axiom/tests/test_matcher.c index 0ab04ff89..9bc198b55 100644 --- a/axiom/tests/test_matcher.c +++ b/axiom/tests/test_matcher.c @@ -63,9 +63,49 @@ static void test_match_single(void) { nr_matcher_destroy(&matcher); } +static void test_match_ex(void) { + char* found; + int len; + nr_matcher_t* matcher = nr_matcher_create(); + + nr_matcher_add_prefix(matcher, "/foo/bar"); + found = nr_matcher_match_ex(matcher, NR_PSTR(""), &len); + tlib_pass_if_null("needle not matched", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("foo"), &len); + tlib_pass_if_null("needle not matched", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/bar"), &len); + tlib_pass_if_null("needle not matched", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/foo/bar/quux"), &len); + tlib_pass_if_str_equal("needle match", "quux", found); + tlib_pass_if_equal("needle match len", 4, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/foo/bar//quux"), &len); + tlib_pass_if_str_equal("needle match", "", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/foo/bar/quux/baz"), &len); + tlib_pass_if_str_equal("needle match", "quux", found); + tlib_pass_if_equal("needle match len", 4, len, int, "%d") + nr_free(found); + + nr_matcher_destroy(&matcher); +} + tlib_parallel_info_t parallel_info = {.suggested_nthreads = 2, .state_size = 0}; void test_main(void* p NRUNUSED) { test_match_multiple(); test_match_single(); + test_match_ex(); } diff --git a/axiom/util_matcher.c b/axiom/util_matcher.c index b69cf7076..e71348eca 100644 --- a/axiom/util_matcher.c +++ b/axiom/util_matcher.c @@ -9,8 +9,15 @@ #include "util_memory.h" #include "util_strings.h" -static void nr_matcher_prefix_dtor(void* prefix, void* userdata NRUNUSED) { - nr_free(prefix); +typedef struct { + char *cp; + int len; +} matcher_prefix; + +static void nr_matcher_prefix_dtor(void* _p, void* userdata NRUNUSED) { + matcher_prefix* p = (matcher_prefix *)_p; + nr_free(p->cp); + nr_free(p); } nr_matcher_t* nr_matcher_create(void) { @@ -31,38 +38,51 @@ void nr_matcher_destroy(nr_matcher_t** matcher_ptr) { nr_realfree((void**)matcher_ptr); } -bool nr_matcher_add_prefix(nr_matcher_t* matcher, const char* prefix) { - size_t i; - char* prefix_lc; - size_t prefix_len; +bool nr_matcher_add_prefix(nr_matcher_t* matcher, const char* str) { + int i; + matcher_prefix* prefix; - if (NULL == matcher || NULL == prefix) { + if (NULL == matcher || NULL == str) { return false; } - prefix_len = nr_strlen(prefix); - while (prefix_len > 0 && '/' == prefix[prefix_len - 1]) { - prefix_len--; + if (NULL == (prefix = nr_calloc(1, sizeof(matcher_prefix)))) { + return false; + } + prefix->len = nr_strlen(str); + while (prefix->len > 0 && '/' == str[prefix->len - 1]) { + prefix->len--; } - prefix_lc = nr_malloc(prefix_len + 2); - for (i = 0; i < prefix_len; i++) { - prefix_lc[i] = nr_tolower(prefix[i]); + prefix->len += 1; // +1 for the trailing '/' + if (NULL == (prefix->cp = nr_malloc(prefix->len+1))) { // +1 for the '\0' + nr_matcher_prefix_dtor(prefix, NULL); + return false; + } + for (i = 0; i < prefix->len; i++) { + prefix->cp[i] = nr_tolower(str[i]); } - prefix_lc[prefix_len] = '/'; - prefix_lc[prefix_len + 1] = '\0'; + prefix->cp[prefix->len-1] = '/'; + prefix->cp[prefix->len] = '\0'; - return nr_vector_push_back(&matcher->prefixes, prefix_lc); + return nr_vector_push_back(&matcher->prefixes, prefix); } -static char* nr_matcher_match_internal(nr_matcher_t* matcher, +#define SET_SAFE(p, v) do {\ + if (NULL != p) *p = (v); \ +} while(0); +static char* nr_matcher_match_internal(nr_matcher_t* matcher, const char* input, + int input_len, + int* match_len, bool core) { size_t i; char* input_lc; char* match = NULL; size_t num_prefixes; + SET_SAFE(match_len, 0); + if (NULL == matcher || NULL == input) { return NULL; } @@ -72,13 +92,13 @@ static char* nr_matcher_match_internal(nr_matcher_t* matcher, for (i = 0; i < num_prefixes; i++) { const char* found; - const char* prefix = nr_vector_get(&matcher->prefixes, i); + const matcher_prefix* prefix = nr_vector_get(&matcher->prefixes, i); - found = nr_strstr(input, prefix); + found = nr_strstr(input, prefix->cp); if (found) { const char* slash; - found += nr_strlen(prefix); + found += prefix->len; if (true == core) { slash = nr_strrchr(found, '/'); } else { @@ -86,12 +106,15 @@ static char* nr_matcher_match_internal(nr_matcher_t* matcher, } if (NULL == slash) { match = nr_strdup(found); + SET_SAFE(match_len, input_len - (found-input)); } else { if (true == core) { - const char* offset = input + nr_strlen(input); + const char* offset = input + input_len; match = nr_strndup(slash + 1, offset - slash); + SET_SAFE(match_len, offset - (slash+1)); } else { match = nr_strndup(found, slash - found); + SET_SAFE(match_len, slash - found); } } break; @@ -102,10 +125,18 @@ static char* nr_matcher_match_internal(nr_matcher_t* matcher, return match; } +char* nr_matcher_match_ex(nr_matcher_t* matcher, const char* input, int input_len, int *match_len) { + return nr_matcher_match_internal(matcher, input, input_len, match_len, false); +} + char* nr_matcher_match(nr_matcher_t* matcher, const char* input) { - return nr_matcher_match_internal(matcher, input, false); + return nr_matcher_match_internal(matcher, input, nr_strlen(input), NULL, false); +} + +char* nr_matcher_match_core_ex(nr_matcher_t* matcher, const char* input, int input_len, int *match_len) { + return nr_matcher_match_internal(matcher, input, input_len, match_len, true); } char* nr_matcher_match_core(nr_matcher_t* matcher, const char* input) { - return nr_matcher_match_internal(matcher, input, true); + return nr_matcher_match_internal(matcher, input, nr_strlen(input), NULL, true); } diff --git a/axiom/util_matcher.h b/axiom/util_matcher.h index 65e22bc10..51aa20e13 100644 --- a/axiom/util_matcher.h +++ b/axiom/util_matcher.h @@ -16,8 +16,10 @@ extern void nr_matcher_destroy(nr_matcher_t** matcher_ptr); extern bool nr_matcher_add_prefix(nr_matcher_t* matcher, const char* prefix); +extern char* nr_matcher_match_ex(nr_matcher_t* matcher, const char* input, int input_len, int* match_len); extern char* nr_matcher_match(nr_matcher_t* matcher, const char* input); +extern char* nr_matcher_match_core_ex(nr_matcher_t* matcher, const char* input, int input_len, int* match_len); extern char* nr_matcher_match_core(nr_matcher_t* matcher, const char* input); #endif diff --git a/axiom/util_strings.h b/axiom/util_strings.h index 7180a450c..395df9f3f 100644 --- a/axiom/util_strings.h +++ b/axiom/util_strings.h @@ -20,7 +20,6 @@ #include #include "util_object.h" -#include "util_memory.h" /* * Purpose : Convert a string to lower case, following USASCII rules, returning @@ -473,35 +472,4 @@ static inline bool nr_striendswith(const char* s, return 0 == nr_stricmp(suffix, pattern); } -/* - * Purpose : Strip the ".php" file extension from a file name - * - * Params : 1. The string filename - * 2. The filename length - * - * Returns : A newly allocated string stripped of the .php extension - * - */ -static inline char* nr_file_basename(char* filename, int filename_len) { - char* retval = NULL; - - if (NULL == filename || 0 >= filename_len) { - return NULL; - } - - if (4 >= filename_len) { - /* if filename_len <= 4, there can't be a ".php" substring to remove. Assume - * the filename does not contain ".php" and return the original filename. */ - return filename; - } - - if (!nr_striendswith(filename, filename_len, NR_PSTR(".php"))) { - return filename; - } - - retval = nr_strndup(filename, filename_len - (sizeof(".php") - 1)); - nr_free(filename); - return retval; -} - #endif /* UTIL_STRINGS_HDR */