[Asterisk-code-review] pbx_functions.c: Manually update ast_str strlen. (asterisk[20])
Joshua Colp
asteriskteam at digium.com
Wed Jul 27 08:07:34 CDT 2022
Joshua Colp has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/18867 )
Change subject: pbx_functions.c: Manually update ast_str strlen.
......................................................................
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
---
M main/pbx_functions.c
M main/pbx_variables.c
2 files changed, 74 insertions(+), 0 deletions(-)
Approvals:
George Joseph: Looks good to me, approved
Joshua Colp: Approved for Submit
diff --git a/main/pbx_functions.c b/main/pbx_functions.c
index 08cc191..081c33f 100644
--- a/main/pbx_functions.c
+++ b/main/pbx_functions.c
@@ -678,6 +678,7 @@
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 45b7ca0..f589b6b 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 @@
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 @@
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 @@
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 @@
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;
}
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18867
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 20
Gerrit-Change-Id: I4e2dba41410f9d4dff61c995d2ca27718248e07f
Gerrit-Change-Number: 18867
Gerrit-PatchSet: 1
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220727/4bd4d29b/attachment-0001.html>
More information about the asterisk-code-review
mailing list