[Asterisk-code-review] pjsip scheduler.c: Fix ao2 usage errors. (asterisk[master])
Richard Mudgett
asteriskteam at digium.com
Mon Apr 9 18:49:57 CDT 2018
Richard Mudgett has uploaded this change for review. ( https://gerrit.asterisk.org/8753
Change subject: pjsip_scheduler.c: Fix ao2 usage errors.
......................................................................
pjsip_scheduler.c: Fix ao2 usage errors.
* Removed several invalid uses of OBJ_NOLOCK. These uses resulted in the
'tasks' container being accessed without a lock in a multi-threaded
environment. A recipe for crashes.
* Removed needlessly obtaining schtd object references. If the caller
providing you a pointer to an object doesn't have a valid reference then
you cannot safely get one from it.
* Getting a ref to 'tasks' when you aren't copying the pointer into
another location is useless. The 'tasks' container pointer is global.
* Removed many unnecessary uses of RAII_VAR.
* Make ast_sip_schedule_task() name parameter const.
ASTERISK_26806
Change-Id: I5c62488e651314e2a1dbc01f5b078a15512d73db
---
M include/asterisk/res_pjsip.h
M res/res_pjsip/pjsip_scheduler.c
2 files changed, 56 insertions(+), 48 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/53/8753/1
diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index 2643998..d3849ad 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -1673,7 +1673,7 @@
*
*/
struct ast_sip_sched_task *ast_sip_schedule_task(struct ast_taskprocessor *serializer,
- int interval, ast_sip_task sip_task, char *name, void *task_data,
+ int interval, ast_sip_task sip_task, const char *name, void *task_data,
enum ast_sip_scheduler_task_flags flags);
/*!
diff --git a/res/res_pjsip/pjsip_scheduler.c b/res/res_pjsip/pjsip_scheduler.c
index e4459da..5b86a79 100644
--- a/res/res_pjsip/pjsip_scheduler.c
+++ b/res/res_pjsip/pjsip_scheduler.c
@@ -60,7 +60,7 @@
enum ast_sip_scheduler_task_flags flags;
/*! the serializer to be used (if any) */
struct ast_taskprocessor *serializer;
- /* A name to be associated with the task */
+ /*! A name to be associated with the task */
char name[0];
};
@@ -113,7 +113,7 @@
delay = schtd->interval - (ast_tvdiff_ms(schtd->last_end, schtd->last_start) % schtd->interval);
}
- schtd->current_scheduler_id = ast_sched_add(scheduler_context, delay, push_to_serializer, (const void *)schtd);
+ schtd->current_scheduler_id = ast_sched_add(scheduler_context, delay, push_to_serializer, schtd);
if (schtd->current_scheduler_id < 0) {
schtd->interval = 0;
ao2_unlock(schtd);
@@ -164,28 +164,28 @@
int ast_sip_sched_task_cancel_by_name(const char *name)
{
- RAII_VAR(struct ast_sip_sched_task *, schtd, NULL, ao2_cleanup);
+ int res;
+ struct ast_sip_sched_task *schtd;
if (ast_strlen_zero(name)) {
return -1;
}
- schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY | OBJ_NOLOCK);
+ schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY);
if (!schtd) {
return -1;
}
- return ast_sip_sched_task_cancel(schtd);
+ res = ast_sip_sched_task_cancel(schtd);
+ ao2_ref(schtd, -1);
+ return res;
}
int ast_sip_sched_task_get_times(struct ast_sip_sched_task *schtd,
struct timeval *queued, struct timeval *last_start, struct timeval *last_end)
{
- if (!ao2_ref_and_lock(schtd)) {
- return -1;
- }
-
+ ao2_lock(schtd);
if (queued) {
memcpy(queued, &schtd->when_queued, sizeof(struct timeval));
}
@@ -195,8 +195,7 @@
if (last_end) {
memcpy(last_end, &schtd->last_end, sizeof(struct timeval));
}
-
- ao2_unlock_and_unref(schtd);
+ ao2_unlock(schtd);
return 0;
}
@@ -204,18 +203,21 @@
int ast_sip_sched_task_get_times_by_name(const char *name,
struct timeval *queued, struct timeval *last_start, struct timeval *last_end)
{
- RAII_VAR(struct ast_sip_sched_task *, schtd, NULL, ao2_cleanup);
+ int res;
+ struct ast_sip_sched_task *schtd;
if (ast_strlen_zero(name)) {
return -1;
}
- schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY | OBJ_NOLOCK);
+ schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY);
if (!schtd) {
return -1;
}
- return ast_sip_sched_task_get_times(schtd, queued, last_start, last_end);
+ res = ast_sip_sched_task_get_times(schtd, queued, last_start, last_end);
+ ao2_ref(schtd, -1);
+ return res;
}
int ast_sip_sched_task_get_name(struct ast_sip_sched_task *schtd, char *name, size_t maxlen)
@@ -224,13 +226,9 @@
return -1;
}
- if (!ao2_ref_and_lock(schtd)) {
- return -1;
- }
-
+ ao2_lock(schtd);
ast_copy_string(name, schtd->name, maxlen);
-
- ao2_unlock_and_unref(schtd);
+ ao2_unlock(schtd);
return 0;
}
@@ -241,9 +239,7 @@
struct timeval since_when;
struct timeval now;
- if (!ao2_ref_and_lock(schtd)) {
- return -1;
- }
+ ao2_lock(schtd);
if (schtd->interval) {
delay = schtd->interval;
@@ -262,50 +258,52 @@
delay = -1;
}
- ao2_unlock_and_unref(schtd);
+ ao2_unlock(schtd);
return delay;
}
int ast_sip_sched_task_get_next_run_by_name(const char *name)
{
- RAII_VAR(struct ast_sip_sched_task *, schtd, NULL, ao2_cleanup);
+ int next_run;
+ struct ast_sip_sched_task *schtd;
if (ast_strlen_zero(name)) {
return -1;
}
- schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY | OBJ_NOLOCK);
+ schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY);
if (!schtd) {
return -1;
}
- return ast_sip_sched_task_get_next_run(schtd);
+ next_run = ast_sip_sched_task_get_next_run(schtd);
+ ao2_ref(schtd, -1);
+ return next_run;
}
int ast_sip_sched_is_task_running(struct ast_sip_sched_task *schtd)
{
- if (!schtd) {
- return 0;
- }
-
- return schtd->is_running;
+ return schtd ? schtd->is_running : 0;
}
int ast_sip_sched_is_task_running_by_name(const char *name)
{
- RAII_VAR(struct ast_sip_sched_task *, schtd, NULL, ao2_cleanup);
+ int is_running;
+ struct ast_sip_sched_task *schtd;
if (ast_strlen_zero(name)) {
return 0;
}
- schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY | OBJ_NOLOCK);
+ schtd = ao2_find(tasks, name, OBJ_SEARCH_KEY);
if (!schtd) {
return 0;
}
- return schtd->is_running;
+ is_running = schtd->is_running;
+ ao2_ref(schtd, -1);
+ return is_running;
}
static void schtd_destructor(void *data)
@@ -321,7 +319,8 @@
}
struct ast_sip_sched_task *ast_sip_schedule_task(struct ast_taskprocessor *serializer,
- int interval, ast_sip_task sip_task, char *name, void *task_data, enum ast_sip_scheduler_task_flags flags)
+ int interval, ast_sip_task sip_task, const char *name, void *task_data,
+ enum ast_sip_scheduler_task_flags flags)
{
#define ID_LEN 13 /* task_deadbeef */
struct ast_sip_sched_task *schtd;
@@ -352,7 +351,7 @@
if (flags & AST_SIP_SCHED_TASK_DATA_AO2) {
ao2_ref(task_data, +1);
}
- res = ast_sched_add(scheduler_context, interval, push_to_serializer, (const void *)schtd);
+ res = ast_sched_add(scheduler_context, interval, push_to_serializer, schtd);
if (res < 0) {
ao2_ref(schtd, -1);
return NULL;
@@ -369,14 +368,14 @@
{
struct ao2_iterator i;
struct ast_sip_sched_task *schtd;
- const char *log_format = ast_logger_get_dateformat();
+ const char *log_format;
struct ast_tm tm;
char queued[32];
char last_start[32];
char next_start[32];
int datelen;
- struct timeval now = ast_tvnow();
- const char *separator = "======================================";
+ struct timeval now;
+ static const char separator[] = "======================================";
switch (cmd) {
case CLI_INIT:
@@ -392,6 +391,9 @@
return CLI_SHOWUSAGE;
}
+ now = ast_tvnow();
+ log_format = ast_logger_get_dateformat();
+
ast_localtime(&now, &tm, NULL);
datelen = ast_strftime(queued, sizeof(queued), log_format, &tm);
@@ -406,12 +408,16 @@
datelen, separator, separator, datelen + 8, separator);
- ao2_ref(tasks, +1);
ao2_rdlock(tasks);
- i = ao2_iterator_init(tasks, 0);
+ i = ao2_iterator_init(tasks, AO2_ITERATOR_DONTLOCK);
while ((schtd = ao2_iterator_next(&i))) {
- int next_run_sec = ast_sip_sched_task_get_next_run(schtd) / 1000;
- struct timeval next = ast_tvadd(now, (struct timeval) {next_run_sec, 0});
+ int next_run_sec;
+ struct timeval next;
+
+ ao2_lock(schtd);
+
+ next_run_sec = ast_sip_sched_task_get_next_run(schtd) / 1000;
+ next = ast_tvadd(now, (struct timeval) {next_run_sec, 0});
ast_localtime(&schtd->when_queued, &tm, NULL);
ast_strftime(queued, sizeof(queued), log_format, &tm);
@@ -434,11 +440,12 @@
datelen, queued, last_start,
next_start,
next_run_sec);
+ ao2_unlock(schtd);
+
ao2_cleanup(schtd);
}
ao2_iterator_destroy(&i);
ao2_unlock(tasks);
- ao2_ref(tasks, -1);
ast_cli(a->fd, "\n");
return CLI_SUCCESS;
@@ -461,8 +468,9 @@
return -1;
}
- tasks = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_RWLOCK, AO2_CONTAINER_ALLOC_OPT_DUPS_REJECT,
- TASK_BUCKETS, ast_sip_sched_task_hash_fn, ast_sip_sched_task_sort_fn, ast_sip_sched_task_cmp_fn);
+ tasks = ao2_container_alloc_hash(AO2_ALLOC_OPT_LOCK_RWLOCK,
+ AO2_CONTAINER_ALLOC_OPT_DUPS_REJECT, TASK_BUCKETS, ast_sip_sched_task_hash_fn,
+ ast_sip_sched_task_sort_fn, ast_sip_sched_task_cmp_fn);
if (!tasks) {
ast_log(LOG_ERROR, "Failed to allocate task container. Aborting load\n");
ast_sched_context_destroy(scheduler_context);
--
To view, visit https://gerrit.asterisk.org/8753
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5c62488e651314e2a1dbc01f5b078a15512d73db
Gerrit-Change-Number: 8753
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180409/c527bc06/attachment-0001.html>
More information about the asterisk-code-review
mailing list