[asterisk-commits] Fix unsafe uses of ast context pointers. (asterisk[11])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Tue Jun 9 06:57:39 CDT 2015
Matt Jordan has submitted this change and it was merged.
Change subject: Fix unsafe uses of ast_context pointers.
......................................................................
Fix unsafe uses of ast_context pointers.
Although ast_context_find, ast_context_find_or_create and
ast_context_destroy perform locking of the contexts table,
any context pointer can become invalid at any time that the
contexts table is unlocked. This change adds locking around
all complete operations involving these functions.
Places where ast_context_find was followed by ast_context_destroy
have been replaced with calls ast_context_destroy_by_name.
ASTERISK-25094 #close
Reported by: Corey Farrell
Change-Id: I1866b6787730c9c4f3f836b6133ffe9c820734fa
---
M apps/app_meetme.c
M channels/chan_iax2.c
M channels/chan_sip.c
M channels/chan_skinny.c
M include/asterisk/pbx.h
M main/features.c
M main/pbx.c
M pbx/pbx_config.c
M tests/test_gosub.c
M tests/test_pbx.c
10 files changed, 91 insertions(+), 74 deletions(-)
Approvals:
Richard Mudgett: Looks good to me, but someone else must approve
Matt Jordan: Looks good to me, approved; Verified
diff --git a/apps/app_meetme.c b/apps/app_meetme.c
index 254f237..2d8d556 100644
--- a/apps/app_meetme.c
+++ b/apps/app_meetme.c
@@ -7347,14 +7347,13 @@
ao2_unlock(trunk);
if (!ast_strlen_zero(trunk->autocontext)) {
- struct ast_context *context;
- context = ast_context_find_or_create(NULL, NULL, trunk->autocontext, sla_registrar);
- if (!context) {
+ if (!ast_context_find_or_create(NULL, NULL, trunk->autocontext, sla_registrar)) {
ast_log(LOG_ERROR, "Failed to automatically find or create "
"context '%s' for SLA!\n", trunk->autocontext);
return -1;
}
- if (ast_add_extension2(context, 0 /* don't replace */, "s", 1,
+
+ if (ast_add_extension(trunk->autocontext, 0 /* don't replace */, "s", 1,
NULL, NULL, slatrunk_app, ast_strdup(trunk->name), ast_free_ptr, sla_registrar)) {
ast_log(LOG_ERROR, "Failed to automatically create extension "
"for trunk '%s'!\n", trunk->name);
@@ -7523,17 +7522,16 @@
ao2_unlock(station);
if (!ast_strlen_zero(station->autocontext)) {
- struct ast_context *context;
struct sla_trunk_ref *trunk_ref;
- context = ast_context_find_or_create(NULL, NULL, station->autocontext, sla_registrar);
- if (!context) {
+
+ if (!ast_context_find_or_create(NULL, NULL, station->autocontext, sla_registrar)) {
ast_log(LOG_ERROR, "Failed to automatically find or create "
"context '%s' for SLA!\n", station->autocontext);
return -1;
}
/* The extension for when the handset goes off-hook.
* exten => station1,1,SLAStation(station1) */
- if (ast_add_extension2(context, 0 /* don't replace */, station->name, 1,
+ if (ast_add_extension(station->autocontext, 0 /* don't replace */, station->name, 1,
NULL, NULL, slastation_app, ast_strdup(station->name), ast_free_ptr, sla_registrar)) {
ast_log(LOG_ERROR, "Failed to automatically create extension "
"for trunk '%s'!\n", station->name);
@@ -7546,7 +7544,7 @@
snprintf(hint, sizeof(hint), "SLA:%s", exten);
/* Extension for this line button
* exten => station1_line1,1,SLAStation(station1_line1) */
- if (ast_add_extension2(context, 0 /* don't replace */, exten, 1,
+ if (ast_add_extension(station->autocontext, 0 /* don't replace */, exten, 1,
NULL, NULL, slastation_app, ast_strdup(exten), ast_free_ptr, sla_registrar)) {
ast_log(LOG_ERROR, "Failed to automatically create extension "
"for trunk '%s'!\n", station->name);
@@ -7554,7 +7552,7 @@
}
/* Hint for this line button
* exten => station1_line1,hint,SLA:station1_line1 */
- if (ast_add_extension2(context, 0 /* don't replace */, exten, PRIORITY_HINT,
+ if (ast_add_extension(station->autocontext, 0 /* don't replace */, exten, PRIORITY_HINT,
NULL, NULL, hint, NULL, NULL, sla_registrar)) {
ast_log(LOG_ERROR, "Failed to automatically create hint "
"for trunk '%s'!\n", station->name);
diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c
index e73b65f..f6708c6 100644
--- a/channels/chan_iax2.c
+++ b/channels/chan_iax2.c
@@ -14783,7 +14783,6 @@
static int __unload_module(void)
{
- struct ast_context *con;
int x;
network_change_event_unsubscribe();
@@ -14863,9 +14862,7 @@
ao2_ref(callno_pool, -1);
ao2_ref(callno_pool_trunk, -1);
- con = ast_context_find(regcontext);
- if (con)
- ast_context_destroy(con, "IAX2");
+ ast_context_destroy_by_name(regcontext, "IAX2");
ast_unload_realtime("iaxpeers");
iax2_tech.capabilities = ast_format_cap_destroy(iax2_tech.capabilities);
diff --git a/channels/chan_sip.c b/channels/chan_sip.c
index c40a4c3..3637ad7 100644
--- a/channels/chan_sip.c
+++ b/channels/chan_sip.c
@@ -19509,8 +19509,7 @@
}
}
- if (stalecontext)
- ast_context_destroy(ast_context_find(stalecontext), "SIP");
+ ast_context_destroy_by_name(stalecontext, "SIP");
}
}
@@ -34869,7 +34868,6 @@
{
struct sip_pvt *p;
struct sip_threadinfo *th;
- struct ast_context *con;
struct ao2_iterator i;
int wait_count;
@@ -35045,10 +35043,7 @@
close(sipsock);
io_context_destroy(io);
ast_sched_context_destroy(sched);
- con = ast_context_find(used_context);
- if (con) {
- ast_context_destroy(con, "SIP");
- }
+ ast_context_destroy_by_name(used_context, "SIP");
ast_unload_realtime("sipregs");
ast_unload_realtime("sippeers");
ast_cc_monitor_unregister(&sip_cc_monitor_callbacks);
diff --git a/channels/chan_skinny.c b/channels/chan_skinny.c
index 58cb564..ef1fda2 100644
--- a/channels/chan_skinny.c
+++ b/channels/chan_skinny.c
@@ -2037,8 +2037,7 @@
}
}
- if (stalecontext)
- ast_context_destroy(ast_context_find(stalecontext), "Skinny");
+ ast_context_destroy_by_name(stalecontext, "Skinny");
}
}
@@ -8014,7 +8013,6 @@
struct skinny_device *d;
struct skinny_line *l;
struct skinny_subchannel *sub;
- struct ast_context *con;
ast_rtp_glue_unregister(&skinny_rtp_glue);
ast_channel_unregister(&skinny_tech);
@@ -8069,9 +8067,7 @@
ast_sched_context_destroy(sched);
}
- con = ast_context_find(used_context);
- if (con)
- ast_context_destroy(con, "Skinny");
+ ast_context_destroy_by_name(used_context, "Skinny");
default_cap = ast_format_cap_destroy(default_cap);
skinny_tech.capabilities = ast_format_cap_destroy(skinny_tech.capabilities);
diff --git a/include/asterisk/pbx.h b/include/asterisk/pbx.h
index 47a787e..40bd6d3 100644
--- a/include/asterisk/pbx.h
+++ b/include/asterisk/pbx.h
@@ -291,7 +291,21 @@
void ast_merge_contexts_and_delete(struct ast_context **extcontexts, struct ast_hashtab *exttable, const char *registrar);
/*!
- * \brief Destroy a context (matches the specified context (or ANY context if NULL)
+ * \brief Destroy a context by name
+ *
+ * \param context Name of the context to destroy
+ * \param registrar who registered it
+ *
+ * You can optionally leave out the registrar parameter. It will find it
+ * based on the context name.
+ *
+ * \retval -1 context not found
+ * \retval 0 Success
+ */
+int ast_context_destroy_by_name(const char *context, const char *registrar);
+
+/*!
+ * \brief Destroy a context (matches the specified context or ANY context if NULL)
*
* \param con context to destroy
* \param registrar who registered it
diff --git a/main/features.c b/main/features.c
index 82b7b2f..c0f4b8d 100644
--- a/main/features.c
+++ b/main/features.c
@@ -5271,6 +5271,7 @@
}
if (manage_parked_call(pu, pfds, nfds, new_pfds, new_nfds, ms)) {
/* Parking is complete for this call so remove it from the parking lot. */
+ ast_wrlock_contexts();
con = ast_context_find(pu->parkinglot->cfg.parking_con);
if (con) {
if (ast_context_remove_extension2(con, pu->parkingexten, 1, NULL, 0)) {
@@ -5278,6 +5279,9 @@
"Whoa, failed to remove the parking extension %s@%s!\n",
pu->parkingexten, pu->parkinglot->cfg.parking_con);
}
+ }
+ ast_unlock_contexts();
+ if (con) {
notify_metermaids(pu->parkingexten, pu->parkinglot->cfg.parking_con,
AST_DEVICE_NOT_INUSE);
} else {
@@ -5493,7 +5497,7 @@
/*! \brief Pickup parked call */
static int parked_call_exec(struct ast_channel *chan, const char *data)
{
- int res;
+ int res = 0;
struct ast_channel *peer = NULL;
struct parkeduser *pu;
struct ast_context *con;
@@ -5567,9 +5571,15 @@
callid = ast_callid_unref(callid);
}
+ ast_wrlock_contexts();
con = ast_context_find(parkinglot->cfg.parking_con);
if (con) {
- if (ast_context_remove_extension2(con, pu->parkingexten, 1, NULL, 0)) {
+ res = ast_context_remove_extension2(con, pu->parkingexten, 1, NULL, 1);
+ }
+ ast_unlock_contexts();
+
+ if (con) {
+ if (res) {
ast_log(LOG_WARNING, "Whoa, failed to remove the extension!\n");
} else {
notify_metermaids(pu->parkingexten, parkinglot->cfg.parking_con, AST_DEVICE_NOT_INUSE);
@@ -6978,7 +6988,6 @@
{
struct parking_dp_context *old_ctx;
struct parking_dp_context *new_ctx;
- struct ast_context *con;
int cmp;
old_ctx = AST_LIST_FIRST(old_map);
@@ -6992,10 +7001,7 @@
cmp = strcmp(old_ctx->context, new_ctx->context);
if (cmp < 0) {
/* New map does not have old map context. */
- con = ast_context_find(old_ctx->context);
- if (con) {
- ast_context_destroy(con, registrar);
- }
+ ast_context_destroy_by_name(old_ctx->context, registrar);
old_ctx = AST_LIST_NEXT(old_ctx, node);
continue;
}
@@ -7011,10 +7017,7 @@
/* Any old contexts left must be dead. */
for (; old_ctx; old_ctx = AST_LIST_NEXT(old_ctx, node)) {
- con = ast_context_find(old_ctx->context);
- if (con) {
- ast_context_destroy(con, registrar);
- }
+ ast_context_destroy_by_name(old_ctx->context, registrar);
}
}
@@ -7245,7 +7248,6 @@
int ast_features_reload(void)
{
- struct ast_context *con;
int res;
ast_mutex_lock(&features_reload_lock);/* Searialize reloading features.conf */
@@ -7258,10 +7260,7 @@
* extension. This is a small window of opportunity on an
* execution chain that is not expected to happen very often.
*/
- con = ast_context_find(parking_con_dial);
- if (con) {
- ast_context_destroy(con, registrar);
- }
+ ast_context_destroy_by_name(parking_con_dial, registrar);
res = load_config(1);
ast_mutex_unlock(&features_reload_lock);
@@ -8687,9 +8686,15 @@
return -1;
}
+ ast_wrlock_contexts();
con = ast_context_find(args->pu->parkinglot->cfg.parking_con);
if (con) {
- if (ast_context_remove_extension2(con, args->pu->parkingexten, 1, NULL, 0)) {
+ res = ast_context_remove_extension2(con, args->pu->parkingexten, 1, NULL, 1);
+ }
+ ast_unlock_contexts();
+
+ if (con) {
+ if (res) {
ast_log(LOG_WARNING, "Whoa, failed to remove the parking extension!\n");
res = -1;
} else {
diff --git a/main/pbx.c b/main/pbx.c
index 42e5f5c..8cff38d 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -8784,9 +8784,9 @@
ast_rdlock_contexts();
local_contexts = &contexts;
tmp = ast_hashtab_lookup(contexts_table, &search);
- ast_unlock_contexts();
if (tmp) {
tmp->refcount++;
+ ast_unlock_contexts();
return tmp;
}
} else { /* local contexts just in a linked list; search there for the new context; slow, linear search, but not frequent */
@@ -8810,11 +8810,13 @@
tmp->refcount = 1;
} else {
ast_log(LOG_ERROR, "Danger! We failed to allocate a context for %s!\n", name);
+ if (!extcontexts) {
+ ast_unlock_contexts();
+ }
return NULL;
}
if (!extcontexts) {
- ast_wrlock_contexts();
tmp->next = *local_contexts;
*local_contexts = tmp;
ast_hashtab_insert_safe(contexts_table, tmp); /*put this context into the tree */
@@ -9710,18 +9712,24 @@
int ast_ignore_pattern(const char *context, const char *pattern)
{
- struct ast_context *con = ast_context_find(context);
+ int ret = 0;
+ struct ast_context *con;
+ ast_rdlock_contexts();
+ con = ast_context_find(context);
if (con) {
struct ast_ignorepat *pat;
for (pat = con->ignorepats; pat; pat = pat->next) {
- if (ast_extension_match(pat->pattern, pattern))
- return 1;
+ if (ast_extension_match(pat->pattern, pattern)) {
+ ret = 1;
+ break;
+ }
}
}
+ ast_unlock_contexts();
- return 0;
+ return ret;
}
/*
@@ -11035,6 +11043,22 @@
}
}
+int ast_context_destroy_by_name(const char *context, const char *registrar)
+{
+ struct ast_context *con;
+ int ret = -1;
+
+ ast_wrlock_contexts();
+ con = ast_context_find(context);
+ if (con) {
+ ast_context_destroy(con, registrar);
+ ret = 0;
+ }
+ ast_unlock_contexts();
+
+ return ret;
+}
+
void ast_context_destroy(struct ast_context *con, const char *registrar)
{
ast_wrlock_contexts();
diff --git a/pbx/pbx_config.c b/pbx/pbx_config.c
index 7114bbc..65dabae 100644
--- a/pbx/pbx_config.c
+++ b/pbx/pbx_config.c
@@ -80,8 +80,6 @@
static char *handle_cli_dialplan_remove_context(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)
{
- struct ast_context *con;
-
switch (cmd) {
case CLI_INIT:
e->command = "dialplan remove context";
@@ -97,16 +95,11 @@
return CLI_SHOWUSAGE;
}
- con = ast_context_find(a->argv[3]);
-
- if (!con) {
- ast_cli(a->fd, "There is no such context as '%s'\n",
- a->argv[3]);
- return CLI_SUCCESS;
+ if (ast_context_destroy_by_name(a->argv[3], NULL)) {
+ ast_cli(a->fd, "There is no such context as '%s'\n", a->argv[3]);
+ return CLI_SUCCESS;
} else {
- ast_context_destroy(con, registrar);
- ast_cli(a->fd, "Removing context '%s'\n",
- a->argv[3]);
+ ast_cli(a->fd, "Removed context '%s'\n", a->argv[3]);
return CLI_SUCCESS;
}
}
diff --git a/tests/test_gosub.c b/tests/test_gosub.c
index 36573fa..3f6b3a7 100644
--- a/tests/test_gosub.c
+++ b/tests/test_gosub.c
@@ -42,10 +42,10 @@
AST_TEST_DEFINE(test_gosub)
{
+#define CONTEXT_NAME "tests_test_gosub_virtual_context"
int res = AST_TEST_PASS, i;
struct ast_channel *chan;
struct ast_str *str;
- struct ast_context *con;
struct testplan {
const char *app;
const char *args;
@@ -119,14 +119,14 @@
}
/* Create our test dialplan */
- if (!(con = ast_context_find_or_create(NULL, NULL, "tests_test_gosub_virtual_context", "test_gosub"))) {
+ if (!ast_context_find_or_create(NULL, NULL, CONTEXT_NAME, "test_gosub")) {
ast_test_status_update(test, "Unable to create test dialplan context");
ast_free(str);
ast_channel_unref(chan);
return AST_TEST_FAIL;
}
- ast_add_extension2(con, 1, "s", 1, NULL, NULL, "NoOp", ast_strdup(""), ast_free_ptr, "test_gosub");
+ ast_add_extension(CONTEXT_NAME, 1, "s", 1, NULL, NULL, "NoOp", ast_strdup(""), ast_free_ptr, "test_gosub");
for (i = 0; i < ARRAY_LEN(testplan); i++) {
if (testplan[i].app == NULL) {
@@ -157,8 +157,8 @@
ast_free(str);
ast_channel_unref(chan);
- ast_context_remove_extension2(con, "s", 1, NULL, 0);
- ast_context_destroy(con, "test_gosub");
+ ast_context_remove_extension(CONTEXT_NAME, "s", 1, NULL);
+ ast_context_destroy(NULL, "test_gosub");
return res;
}
diff --git a/tests/test_pbx.c b/tests/test_pbx.c
index 5e2e232..bb5d8e8 100644
--- a/tests/test_pbx.c
+++ b/tests/test_pbx.c
@@ -198,7 +198,6 @@
*/
struct {
const char * context_string;
- struct ast_context *context;
} contexts[] = {
{ TEST_PATTERN, },
{ TEST_PATTERN_INCLUDE, },
@@ -267,7 +266,7 @@
*/
for (i = 0; i < ARRAY_LEN(contexts); ++i) {
- if (!(contexts[i].context = ast_context_find_or_create(NULL, NULL, contexts[i].context_string, registrar))) {
+ if (!ast_context_find_or_create(NULL, NULL, contexts[i].context_string, registrar)) {
ast_test_status_update(test, "Failed to create context %s\n", contexts[i].context_string);
res = AST_TEST_FAIL;
goto cleanup;
@@ -319,11 +318,7 @@
}
cleanup:
- for (i = 0; i < ARRAY_LEN(contexts); ++i) {
- if (contexts[i].context) {
- ast_context_destroy(contexts[i].context, registrar);
- }
- }
+ ast_context_destroy(NULL, registrar);
return res;
}
--
To view, visit https://gerrit.asterisk.org/481
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1866b6787730c9c4f3f836b6133ffe9c820734fa
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: 11
Gerrit-Owner: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-commits
mailing list