[Asterisk-code-review] various: fix and test a double deref on a scheduled delete of an exec... (asterisk[16])
Michael Bradeen
asteriskteam at digium.com
Fri Dec 10 11:58:22 CST 2021
Michael Bradeen has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/17644 )
Change subject: various: fix and test a double deref on a scheduled delete of an executing call back.
......................................................................
various: fix and test a double deref on a scheduled delete of an
executing call back.
sched: Avoid a double deref when AST_SCHED_DEL_UNREF is called on an
executing call-back. This is done by adding a new variable 'rescheduled'
to the struct sched which is set in ast_sched_runq and checked in
ast_sched_del. ast_sched_del will now return a new possible value -2 if
called on an executing call-back with rescheduled set.
AST_SCHED_DEL_UNREF is also updated to look for the -2 in which case it
will not throw a warning or invoke refcall.
test_sched: Add a new unit test sched_test_freebird that will check the
reference count in the resolved scenario.
ASTERISK-29698
Change-Id: Icfb16b3acbc29cf5b4cef74183f7531caaefe21d
---
M include/asterisk/sched.h
M main/sched.c
M tests/test_sched.c
3 files changed, 154 insertions(+), 5 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/44/17644/1
diff --git a/include/asterisk/sched.h b/include/asterisk/sched.h
index bbef914..cd6d2ad 100644
--- a/include/asterisk/sched.h
+++ b/include/asterisk/sched.h
@@ -79,13 +79,13 @@
*/
#define AST_SCHED_DEL_UNREF(sched, id, refcall) \
do { \
- int _count = 0, _id; \
- while ((_id = id) > -1 && ast_sched_del(sched, _id) && ++_count < 10) { \
+ int _count = 0, _id, _ret = 0; \
+ while ((_id = id) > -1 && (( _ret = ast_sched_del(sched, _id)) == -1) && ++_count < 10) { \
usleep(1); \
} \
if (_count == 10) { \
ast_log(LOG_WARNING, "Unable to cancel schedule ID %d. This is probably a bug (%s: %s, line %d).\n", _id, __FILE__, __PRETTY_FUNCTION__, __LINE__); \
- } else if (_id > -1) { \
+ } else if (_id > -1 && _ret >-2) { \
refcall; \
id = -1; \
} \
@@ -293,6 +293,7 @@
* \param id ID of the scheduled item to delete
*
* \retval -1 on failure
+ * \retval -2 event was running and not rescheduled
* \retval 0 on success
*/
int ast_sched_del(struct ast_sched_context *con, int id) attribute_warn_unused_result;
diff --git a/main/sched.c b/main/sched.c
index e3a7d30..b759390 100644
--- a/main/sched.c
+++ b/main/sched.c
@@ -98,6 +98,8 @@
ast_cond_t cond;
/*! Indication that a running task was deleted. */
unsigned int deleted:1;
+ /*! Indication that a running task was rescheduled. */
+ unsigned int rescheduled:1;
};
struct sched_thread {
@@ -611,6 +613,7 @@
{
struct sched *s = NULL;
int *last_id = ast_threadstorage_get(&last_del_id, sizeof(int));
+ int res = 0;
DEBUG(ast_debug(1, "ast_sched_del(%d)\n", id));
@@ -646,6 +649,13 @@
ast_cond_wait(&s->cond, &con->lock);
}
/* Do not sched_release() here because ast_sched_runq() will do it */
+ /* This was not rescheduled so the caller of ast_sched_del can not remove any
+ * references as they already were.
+ */
+ if (!s->rescheduled) {
+ res = -2;
+ }
+ sched_release(con, s);
}
}
@@ -668,7 +678,7 @@
return -1;
}
- return 0;
+ return res;
}
void ast_sched_report(struct ast_sched_context *con, struct ast_str **buf, struct ast_cb_names *cbnames)
@@ -793,7 +803,9 @@
con->currently_executing = NULL;
ast_cond_signal(¤t->cond);
- if (res && !current->deleted) {
+ if (current->deleted) {
+ current->rescheduled = res ? 1 : 0;
+ } else if (res) {
/*
* If they return non-zero, we should schedule them to be
* run again.
diff --git a/tests/test_sched.c b/tests/test_sched.c
index e995c2c..90c461f 100644
--- a/tests/test_sched.c
+++ b/tests/test_sched.c
@@ -37,6 +37,7 @@
#include "asterisk/sched.h"
#include "asterisk/test.h"
#include "asterisk/cli.h"
+#include "asterisk/astobj2.h"
static int sched_cb(const void *data)
{
@@ -336,6 +337,139 @@
return CLI_SUCCESS;
}
+struct test_obj {
+ ast_mutex_t lock;
+ ast_cond_t cond;
+ int scheduledCBstarted;
+ int id;
+};
+
+static void test_obj_cleanup(void *data)
+{
+ struct test_obj *obj = data;
+ ast_mutex_destroy(&obj->lock);
+ ast_cond_destroy(&obj->cond);
+}
+
+//static void context_cleanup(void *data)
+//{
+// struct ast_sched_context *c = data;
+// ast_sched_context_destroy(c);
+//}
+
+static int lockingcb(const void *data)
+{
+ struct test_obj *obj = (struct test_obj *)data;
+
+ ast_mutex_lock(&obj->lock);
+
+ obj->scheduledCBstarted = 1;
+ ast_cond_signal(&obj->cond);
+
+ ast_mutex_unlock(&obj->lock);
+
+ ao2_ref(obj, -1);
+ usleep(3000);
+
+ return 0;
+}
+
+AST_TEST_DEFINE(sched_test_freebird)
+{
+
+ //RAII_VAR(struct test_obj *, obj, ao2_alloc(sizeof(*obj), test_obj_cleanup), ao2_cleanup);
+ //RAII_VAR(struct ast_sched_context *, con, ast_sched_context_create(), context_cleanup), ao2_cleanup);
+
+ struct ast_sched_context *con;
+ enum ast_test_result_state res = AST_TEST_FAIL;
+ struct test_obj *obj;
+ int refs;
+
+ switch (cmd) {
+ case TEST_INIT:
+ info->name = "sched_test_freebird";
+ info->category = "/main/sched/";
+ info->summary = "Test deadlock avoidance and double-unref";
+ info->description =
+ "This test forces a thread conflict issue on res mem sorcery.";
+ return AST_TEST_NOT_RUN;
+ case TEST_EXECUTE:
+ res = AST_TEST_PASS;
+ break;
+ }
+
+ if (!(obj = ao2_alloc(sizeof(struct test_obj), test_obj_cleanup))) {
+ ast_test_status_update(test,
+ "ao2_alloc() did not return an object\n");
+ return AST_TEST_FAIL;
+ }
+
+ /* This additional reference is to ensure that the object isn't destroyed prematurely
+ * in a case where it is unreffed an additional time.
+ */
+ ao2_ref(obj, +1);
+ obj->scheduledCBstarted = 0;
+
+ if (!(con = ast_sched_context_create())) {
+ ast_test_status_update(test,
+ "ast_sched_context_create() did not return a context\n");
+ return AST_TEST_FAIL;
+ }
+
+ if(ast_sched_start_thread(con)) {
+ ast_test_status_update(test, "Failed to start test thread\n");
+ ast_sched_context_destroy(con);
+ return AST_TEST_FAIL;
+ }
+
+ ao2_bump(obj);
+ if ((obj->id = ast_sched_add(con, 0, lockingcb, obj)) == -1) {
+ ast_test_status_update(test, "Failed to add scheduler entry\n");
+ ast_sched_context_destroy(con);
+ return AST_TEST_FAIL;
+ }
+
+ ast_mutex_lock(&obj->lock);
+ while(obj->scheduledCBstarted == 0) {
+ ast_cond_wait(&obj->cond,&obj->lock);
+ }
+ ast_mutex_unlock(&obj->lock);
+
+ ast_test_status_update(test, "Received signal, calling Scedule and UNREF\n");
+ ast_test_status_update(test, "ID: %d\n", obj->id);
+ AST_SCHED_DEL_UNREF(con, obj->id, ao2_ref(obj, -1));
+
+/* do{
+ int _count = 0, _id, ret;
+ while ((_id = obj->id) > -1 && ((ret = ast_sched_del(con, _id)) ==-1) && ++_count < 10) {
+ usleep(1);
+ }
+ if (_count == 10) {
+ ast_log(LOG_WARNING, "Unable to cancel schedule ID %d. This is probably a bug (%s: %s, line %d).\n", _id, __FILE__, __PRETTY_FUNCTION__, __LINE__);
+ } else if (_id > -1 && ret >-2) {
+ //refcall;
+ ao2_ref(obj, -1);
+ obj->id = -1;
+ }
+ } while(0); */
+
+ refs = ao2_ref(obj, 0);
+
+ switch(refs){
+ case 2:
+ ast_test_status_update(test, "Correct number of references '2'\n");
+ break;
+ default:
+ ast_test_status_update(test, "Inorrect number of references '%d'\n",
+ refs);
+ res = AST_TEST_FAIL;
+ break;
+ }
+
+ ast_sched_context_destroy(con);
+ return res;
+}
+
static struct ast_cli_entry cli_sched[] = {
AST_CLI_DEFINE(handle_cli_sched_bench, "Benchmark ast_sched add/del performance"),
};
@@ -343,6 +477,7 @@
static int unload_module(void)
{
AST_TEST_UNREGISTER(sched_test_order);
+ AST_TEST_UNREGISTER(sched_test_freebird);
ast_cli_unregister_multiple(cli_sched, ARRAY_LEN(cli_sched));
return 0;
}
@@ -350,6 +485,7 @@
static int load_module(void)
{
AST_TEST_REGISTER(sched_test_order);
+ AST_TEST_REGISTER(sched_test_freebird);
ast_cli_register_multiple(cli_sched, ARRAY_LEN(cli_sched));
return AST_MODULE_LOAD_SUCCESS;
}
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/17644
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Icfb16b3acbc29cf5b4cef74183f7531caaefe21d
Gerrit-Change-Number: 17644
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211210/aa8fc71e/attachment-0001.html>
More information about the asterisk-code-review
mailing list