From a2799554d25d74eeb5dfb8bc8e52887fb27637fb Mon Sep 17 00:00:00 2001 From: Naveen Albert Date: Sat, 23 Jul 2022 22:17:28 +0000 Subject: [PATCH] pbx_functions.c: Manually update ast_str strlen. When ast_func_read2 is used to read a function using its read function (as opposed to a native ast_str read2 function), the result is copied directly by the function into the ast_str buffer. As a result, the ast_str length remains initialized to 0, which is a bug because this is not the real string length. This can cascade and have issues elsewhere, such as when reading substrings of functions that only register read as opposed to read2 callbacks. In this case, since reading ast_str_strlen returns 0, the returned substring is empty as opposed to the actual substring. This has caused the ast_str family of functions to behave inconsistently and erroneously, in contrast to the pbx_variables substitution functions which work correctly. This fixes this issue by manually updating the ast_str length when the result is copied directly into the ast_str buffer. Additionally, an assertion and a unit test that previously exposed these issues are added, now that the issue is fixed. ASTERISK-29966 #close Change-Id: I4e2dba41410f9d4dff61c995d2ca27718248e07f --- main/pbx_functions.c | 1 + main/pbx_variables.c | 73 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/main/pbx_functions.c b/main/pbx_functions.c index 08cc191f5f..081c33f6ed 100644 --- a/main/pbx_functions.c +++ b/main/pbx_functions.c @@ -678,6 +678,7 @@ int ast_func_read2(struct ast_channel *chan, const char *function, struct ast_st ast_str_make_space(str, maxsize); } res = acfptr->read(chan, copy, args, ast_str_buffer(*str), maxsize); + ast_str_update(*str); /* Manually set the string length */ } if (acfptr->mod && u) { __ast_module_user_remove(acfptr->mod, u); diff --git a/main/pbx_variables.c b/main/pbx_variables.c index 45b7ca0984..f589b6bce0 100644 --- a/main/pbx_variables.c +++ b/main/pbx_variables.c @@ -40,6 +40,7 @@ #include "asterisk/paths.h" #include "asterisk/pbx.h" #include "asterisk/stasis_channels.h" +#include "asterisk/test.h" #include "pbx_private.h" /*** DOCUMENTATION @@ -189,6 +190,8 @@ static const char *ast_str_substring(struct ast_str *value, int offset, int leng lr = ast_str_strlen(value); /* compute length after copy, so we never go out of the workspace */ + ast_assert(lr == strlen(ast_str_buffer(value))); /* ast_str_strlen should always agree with strlen */ + /* Quick check if no need to do anything */ if (offset == 0 && length >= lr) /* take the whole string */ return ast_str_buffer(value); @@ -1292,6 +1295,74 @@ void pbx_builtin_clear_globals(void) ast_rwlock_unlock(&globalslock); } +#ifdef TEST_FRAMEWORK +AST_TEST_DEFINE(test_variable_substrings) +{ + int i, res = AST_TEST_PASS; + struct ast_channel *chan; /* dummy channel */ + struct ast_str *str; /* fancy string for holding comparing value */ + + const char *test_strings[][5] = { + {"somevaluehere", "CALLERID(num):0:25", "somevaluehere"}, + {"somevaluehere", "CALLERID(num):0:5", "somev"}, + {"somevaluehere", "CALLERID(num):4:5", "value"}, + {"somevaluehere", "CALLERID(num):0:-4", "somevalue"}, + {"somevaluehere", "CALLERID(num):-4", "here"}, + }; + + switch (cmd) { + case TEST_INIT: + info->name = "variable_substrings"; + info->category = "/main/pbx/"; + info->summary = "Test variable substring resolution"; + info->description = "Verify that variable substrings are calculated correctly"; + return AST_TEST_NOT_RUN; + case TEST_EXECUTE: + break; + } + + if (!(chan = ast_dummy_channel_alloc())) { + ast_test_status_update(test, "Unable to allocate dummy channel\n"); + return AST_TEST_FAIL; + } + + if (!(str = ast_str_create(64))) { + ast_test_status_update(test, "Unable to allocate dynamic string buffer\n"); + ast_channel_release(chan); + return AST_TEST_FAIL; + } + + for (i = 0; i < ARRAY_LEN(test_strings); i++) { + char substituted[512], tmp[512] = ""; + + ast_set_callerid(chan, test_strings[i][0], NULL, test_strings[i][0]); + + snprintf(tmp, sizeof(tmp), "${%s}", test_strings[i][1]); + + /* test ast_str_substitute_variables */ + ast_str_substitute_variables(&str, 0, chan, tmp); + ast_debug(1, "Comparing STR %s with %s\n", ast_str_buffer(str), test_strings[i][2]); + if (strcmp(test_strings[i][2], ast_str_buffer(str))) { + ast_test_status_update(test, "Format string '%s' substituted to '%s' using str sub. Expected '%s'.\n", test_strings[i][0], ast_str_buffer(str), test_strings[i][2]); + res = AST_TEST_FAIL; + } + + /* test pbx_substitute_variables_helper */ + pbx_substitute_variables_helper(chan, tmp, substituted, sizeof(substituted) - 1); + ast_debug(1, "Comparing PBX %s with %s\n", substituted, test_strings[i][2]); + if (strcmp(test_strings[i][2], substituted)) { + ast_test_status_update(test, "Format string '%s' substituted to '%s' using pbx sub. Expected '%s'.\n", test_strings[i][0], substituted, test_strings[i][2]); + res = AST_TEST_FAIL; + } + } + ast_free(str); + + ast_channel_release(chan); + + return res; +} +#endif + static struct ast_cli_entry vars_cli[] = { AST_CLI_DEFINE(handle_show_globals, "Show global dialplan variables"), AST_CLI_DEFINE(handle_show_chanvar, "Show channel variables"), @@ -1306,6 +1377,7 @@ static void unload_pbx_variables(void) ast_unregister_application("Set"); ast_unregister_application("MSet"); pbx_builtin_clear_globals(); + AST_TEST_UNREGISTER(test_variable_substrings); } int load_pbx_variables(void) @@ -1316,6 +1388,7 @@ int load_pbx_variables(void) res |= ast_register_application2("Set", pbx_builtin_setvar, NULL, NULL, NULL); res |= ast_register_application2("MSet", pbx_builtin_setvar_multiple, NULL, NULL, NULL); ast_register_cleanup(unload_pbx_variables); + AST_TEST_REGISTER(test_variable_substrings); return res; }