From f0ab508d4223416393e702209e8293fd3d4c589b Mon Sep 17 00:00:00 2001 From: Nanang Izzuddin Date: Mon, 6 Jan 2025 17:44:13 +0700 Subject: [PATCH 1/4] Fix possible deadlock in conference (audio & video) --- pjmedia/src/pjmedia/conference.c | 52 ++++++++++++++++++++---------- pjmedia/src/pjmedia/vid_conf.c | 54 ++++++++++++++++++++++---------- 2 files changed, 74 insertions(+), 32 deletions(-) diff --git a/pjmedia/src/pjmedia/conference.c b/pjmedia/src/pjmedia/conference.c index 80bfa66c84..9f763c2bcf 100644 --- a/pjmedia/src/pjmedia/conference.c +++ b/pjmedia/src/pjmedia/conference.c @@ -340,34 +340,53 @@ static op_entry* get_free_op_entry(pjmedia_conf *conf) static void handle_op_queue(pjmedia_conf *conf) { - op_entry *op, *next_op; + /* The queue may grow while mutex is released, better put a limit? */ + enum { MAX_PROCESSED_OP = 100 }; + int i = 0; - op = conf->op_queue->next; - while (op != conf->op_queue) { - next_op = op->next; + while (i++ < MAX_PROCESSED_OP) { + op_entry *op; + op_type type; + op_param param; + + pj_mutex_lock(conf->mutex); + + /* Stop when queue empty */ + if (pj_list_empty(conf->op_queue)) { + pj_mutex_unlock(conf->mutex); + break; + } + + /* Copy op */ + op = conf->op_queue->next; + type = op->type; + param = op->param; + + /* Free op */ pj_list_erase(op); + op->type = OP_UNKNOWN; + pj_list_push_back(conf->op_queue_free, op); + + pj_mutex_unlock(conf->mutex); - switch(op->type) { + /* Process op */ + switch(type) { case OP_ADD_PORT: - op_add_port(conf, &op->param); + op_add_port(conf, ¶m); break; case OP_REMOVE_PORT: - op_remove_port(conf, &op->param); + op_remove_port(conf, ¶m); break; case OP_CONNECT_PORTS: - op_connect_ports(conf, &op->param); + op_connect_ports(conf, ¶m); break; case OP_DISCONNECT_PORTS: - op_disconnect_ports(conf, &op->param); + op_disconnect_ports(conf, ¶m); break; default: pj_assert(!"Invalid sync-op in conference"); break; } - - op->type = OP_UNKNOWN; - pj_list_push_back(conf->op_queue_free, op); - op = next_op; } } @@ -1380,7 +1399,7 @@ static void op_disconnect_ports(pjmedia_conf *conf, { unsigned src_slot, sink_slot; struct conf_port *src_port = NULL, *dst_port = NULL; - int i; + unsigned i; /* Ports must be valid. */ src_slot = prm->disconnect_ports.src; @@ -1722,7 +1741,10 @@ static void op_remove_port(pjmedia_conf *conf, const op_param *prm) } /* Remove the port. */ + pj_mutex_lock(conf->mutex); conf->ports[port] = NULL; + pj_mutex_unlock(conf->mutex); + if (!conf_port->is_new) --conf->port_cnt; @@ -2418,9 +2440,7 @@ static pj_status_t get_frame(pjmedia_port *this_port, */ if (!pj_list_empty(conf->op_queue)) { pj_log_push_indent(); - pj_mutex_lock(conf->mutex); handle_op_queue(conf); - pj_mutex_unlock(conf->mutex); pj_log_pop_indent(); } diff --git a/pjmedia/src/pjmedia/vid_conf.c b/pjmedia/src/pjmedia/vid_conf.c index 9634c39267..c4697ffaae 100644 --- a/pjmedia/src/pjmedia/vid_conf.c +++ b/pjmedia/src/pjmedia/vid_conf.c @@ -202,37 +202,56 @@ static op_entry* get_free_op_entry(pjmedia_vid_conf *conf) static void handle_op_queue(pjmedia_vid_conf *conf) { - op_entry *op, *next_op; - - op = conf->op_queue->next; - while (op != conf->op_queue) { - next_op = op->next; + /* The queue may grow while mutex is released, better put a limit? */ + enum { MAX_PROCESSED_OP = 100 }; + int i = 0; + + while (i++ < MAX_PROCESSED_OP) { + op_entry* op; + op_type type; + op_param param; + + pj_mutex_lock(conf->mutex); + + /* Stop when queue empty */ + if (pj_list_empty(conf->op_queue)) { + pj_mutex_unlock(conf->mutex); + break; + } + + /* Copy op */ + op = conf->op_queue->next; + type = op->type; + param = op->param; + + /* Free op */ pj_list_erase(op); + op->type = OP_UNKNOWN; + pj_list_push_back(conf->op_queue_free, op); + + pj_mutex_unlock(conf->mutex); - switch(op->type) { - case OP_ADD_PORT: - op_add_port(conf, &op->param); + /* Process op */ + switch (type) { + case OP_ADD_PORT: + op_add_port(conf, ¶m); break; case OP_REMOVE_PORT: - op_remove_port(conf, &op->param); + op_remove_port(conf, ¶m); break; case OP_CONNECT_PORTS: - op_connect_ports(conf, &op->param); + op_connect_ports(conf, ¶m); break; case OP_DISCONNECT_PORTS: - op_disconnect_ports(conf, &op->param); + op_disconnect_ports(conf, ¶m); break; case OP_UPDATE_PORT: - op_update_port(conf, &op->param); + op_update_port(conf, ¶m); break; default: pj_assert(!"Invalid sync-op in video conference"); break; } - - op->type = OP_UNKNOWN; - pj_list_push_back(conf->op_queue_free, op); - op = next_op; } } @@ -708,7 +727,10 @@ static void op_remove_port(pjmedia_vid_conf *vid_conf, } /* Remove the port. */ + pj_mutex_lock(vid_conf->mutex); vid_conf->ports[slot] = NULL; + pj_mutex_unlock(vid_conf->mutex); + if (!cport->is_new) --vid_conf->port_cnt; From 5da37720918641819282d7cd5d87897e35a3c417 Mon Sep 17 00:00:00 2001 From: Nanang Izzuddin Date: Mon, 6 Jan 2025 17:52:02 +0700 Subject: [PATCH 2/4] Forgot to remove mutex around handle_op_queue() in vid conf. --- pjmedia/src/pjmedia/vid_conf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pjmedia/src/pjmedia/vid_conf.c b/pjmedia/src/pjmedia/vid_conf.c index c4697ffaae..9e9d1e344b 100644 --- a/pjmedia/src/pjmedia/vid_conf.c +++ b/pjmedia/src/pjmedia/vid_conf.c @@ -1108,9 +1108,9 @@ static void on_clock_tick(const pj_timestamp *now, void *user_data) * the clock such as connect, disonnect, remove, update. */ if (!pj_list_empty(vid_conf->op_queue)) { - pj_mutex_lock(vid_conf->mutex); + pj_log_push_indent(); handle_op_queue(vid_conf); - pj_mutex_unlock(vid_conf->mutex); + pj_log_pop_indent(); } /* No mutex from this point! Otherwise it may cause deadlock as From 4920150acd04eee33dc5b133b8822c00970cb33e Mon Sep 17 00:00:00 2001 From: Nanang Izzuddin Date: Mon, 6 Jan 2025 17:54:33 +0700 Subject: [PATCH 3/4] Minor indentation fix --- pjmedia/src/pjmedia/vid_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pjmedia/src/pjmedia/vid_conf.c b/pjmedia/src/pjmedia/vid_conf.c index 9e9d1e344b..81ae52501c 100644 --- a/pjmedia/src/pjmedia/vid_conf.c +++ b/pjmedia/src/pjmedia/vid_conf.c @@ -233,7 +233,7 @@ static void handle_op_queue(pjmedia_vid_conf *conf) /* Process op */ switch (type) { - case OP_ADD_PORT: + case OP_ADD_PORT: op_add_port(conf, ¶m); break; case OP_REMOVE_PORT: From 37678e736b95eef6f01aed1c1fbde4d286a5e2a3 Mon Sep 17 00:00:00 2001 From: Nanang Izzuddin Date: Mon, 6 Jan 2025 18:37:22 +0700 Subject: [PATCH 4/4] Revert int -> unsigned (was to fix warning, turns out negative value is expected). --- pjmedia/src/pjmedia/conference.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pjmedia/src/pjmedia/conference.c b/pjmedia/src/pjmedia/conference.c index 9f763c2bcf..0e6277a219 100644 --- a/pjmedia/src/pjmedia/conference.c +++ b/pjmedia/src/pjmedia/conference.c @@ -1399,7 +1399,7 @@ static void op_disconnect_ports(pjmedia_conf *conf, { unsigned src_slot, sink_slot; struct conf_port *src_port = NULL, *dst_port = NULL; - unsigned i; + int i; /* Ports must be valid. */ src_slot = prm->disconnect_ports.src;