[Asterisk-code-review] pjsip scheduler.c: Fix ao2 usage errors. (asterisk[13])

Jenkins2 asteriskteam at digium.com
Thu Apr 12 10:10:30 CDT 2018


Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/8741 )

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(-)

Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Jenkins2: Approved for Submit



diff --git a/include/asterisk/res_pjsip.h b/include/asterisk/res_pjsip.h
index 8cb428f..d091f9a 100644
--- a/include/asterisk/res_pjsip.h
+++ b/include/asterisk/res_pjsip.h
@@ -1702,7 +1702,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 7520db8..43a9f59 100644
--- a/res/res_pjsip/pjsip_scheduler.c
+++ b/res/res_pjsip/pjsip_scheduler.c
@@ -62,7 +62,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];
 };
 
@@ -115,7 +115,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);
@@ -166,28 +166,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));
 	}
@@ -197,8 +197,7 @@
 	if (last_end) {
 		memcpy(last_end, &schtd->last_end, sizeof(struct timeval));
 	}
-
-	ao2_unlock_and_unref(schtd);
+	ao2_unlock(schtd);
 
 	return 0;
 }
@@ -206,18 +205,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)
@@ -226,13 +228,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;
 }
@@ -243,9 +241,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;
@@ -264,50 +260,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)
@@ -323,7 +321,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;
@@ -354,7 +353,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;
@@ -371,14 +370,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:
@@ -394,6 +393,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);
 
@@ -408,12 +410,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);
@@ -436,11 +442,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;
@@ -463,8 +470,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/8741
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: merged
Gerrit-Change-Id: I5c62488e651314e2a1dbc01f5b078a15512d73db
Gerrit-Change-Number: 8741
Gerrit-PatchSet: 3
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180412/6c534518/attachment-0001.html>


More information about the asterisk-code-review mailing list