Skip to content

Commit fa7f7e2

Browse files
Copilotnanangizz
Copilot
and
nanangizz
authored
Fix port ID reuse when quickly removing and adding conference bridge ports (#4722)
* Initial plan * Fix port ID reuse by freeing slot after removal callback Move conf slot freeing from op_remove_port() to new op_remove_port2() function that is called after the callback in handle_op_queue(). This ensures port IDs remain unique - a slot won't be reused until after the removal callback has been invoked. Changes: - Added op_remove_port2() to free the conf slot after callback - Modified handle_op_queue() to call op_remove_port2() after callback for REMOVE_PORT ops - Removed slot freeing code from op_remove_port() (moved to op_remove_port2()) - Updated log messages to distinguish "Removing" vs "Removed" states Co-authored-by: nanangizz <[email protected]> * Apply slot freeing after callback to synchronous removals too Also call op_remove_port2() after callback in synchronous port removal cases (pjmedia_conf_destroy and immediate removal in pjmedia_conf_remove_port). This ensures consistent behavior whether removal is queued or synchronous. Co-authored-by: nanangizz <[email protected]> * Address code review feedback: fix mutex and add warning log - Read port_cnt before releasing mutex to avoid inconsistent log output - Add warning log when port is already freed to diagnose unexpected scenarios Co-authored-by: nanangizz <[email protected]> * Apply port ID reuse fix to conf_thread.c and vid_conf.c Extended the fix for port ID reuse to conf_thread.c and vid_conf.c: - Added op_remove_port2() function to both files - Modified handle_op_queue() to call op_remove_port2() after callback - Updated synchronous removal paths in destroy functions - Moved slot freeing from op_remove_port() to op_remove_port2() This ensures port IDs remain unique across all conference bridge implementations. Co-authored-by: nanangizz <[email protected]> * Fix heap-use-after-free in op_remove_port2 Moved port count decrement and active_listener cleanup from op_remove_port2() to op_remove_port() to happen BEFORE the port is destroyed. This prevents accessing freed memory in op_remove_port2(). The issue was that op_remove_port() would destroy the conf_port structure, then op_remove_port2() would try to access conf_port->is_new from the freed structure, causing a heap-use-after-free error detected by AddressSanitizer. Fixed in all three conference bridge implementations: - conference.c - conf_thread.c - vid_conf.c Co-authored-by: nanangizz <[email protected]> * Move port destruction to op_remove_port2 for consistent port count Moved port destruction and port count decrement from op_remove_port() to op_remove_port2() so that these operations happen after the callback is invoked. This ensures the port count remains consistent throughout the removal process. Now the flow is: 1. op_remove_port() - Disconnects port and prepares for removal 2. Callback is invoked 3. op_remove_port2() - Frees slot, updates port count, and destroys port This prevents the port count from changing before the callback, which could confuse applications monitoring port counts. Applied to all three conference bridge implementations. Co-authored-by: nanangizz <[email protected]> * Move active_listener cleanup to op_remove_port for clarity Moved the active_listener array cleanup from op_remove_port2() to op_remove_port() for better code clarity. This operation doesn't need to happen after the callback and doesn't access any freed memory, so it's safer and clearer to do it in op_remove_port(). Now op_remove_port2() is focused on only the very final steps: - Free the slot - Update port count - Destroy the port This makes the code easier to understand and maintain. Co-authored-by: nanangizz <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: nanangizz <[email protected]>
1 parent 9ffdbd7 commit fa7f7e2

File tree

3 files changed

+146
-36
lines changed

3 files changed

+146
-36
lines changed

‎pjmedia/src/pjmedia/conf_thread.c‎

Lines changed: 52 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,8 @@ static pj_status_t op_add_port(pjmedia_conf *conf,
477477
const pjmedia_conf_op_param *prm);
478478
static pj_status_t op_remove_port(pjmedia_conf *conf,
479479
const pjmedia_conf_op_param *prm);
480+
static void op_remove_port2(pjmedia_conf *conf,
481+
const pjmedia_conf_op_param *prm);
480482
static pj_status_t op_connect_ports(pjmedia_conf *conf,
481483
const pjmedia_conf_op_param *prm);
482484
static pj_status_t op_disconnect_ports(pjmedia_conf *conf,
@@ -557,6 +559,11 @@ static void handle_op_queue(pjmedia_conf *conf)
557559
(*conf->cb)(&info);
558560
pj_log_pop_indent();
559561
}
562+
563+
/* Free the conf slot after callback for remove port operation */
564+
if (type == PJMEDIA_CONF_OP_REMOVE_PORT) {
565+
op_remove_port2(conf, &param);
566+
}
560567
}
561568
}
562569

@@ -1298,6 +1305,8 @@ PJ_DEF(pj_status_t) pjmedia_conf_destroy( pjmedia_conf *conf )
12981305
(*conf->cb)(&op_info);
12991306
pj_log_pop_indent();
13001307
}
1308+
/* Free the conf slot after callback */
1309+
op_remove_port2(conf, &oprm);
13011310
}
13021311
}
13031312

@@ -2333,6 +2342,9 @@ PJ_DEF(pj_status_t) pjmedia_conf_remove_port( pjmedia_conf *conf,
23332342
pj_log_pop_indent();
23342343
}
23352344

2345+
/* Free the conf slot after callback */
2346+
op_remove_port2(conf, &prm);
2347+
23362348
pj_log_pop_indent();
23372349
return PJ_SUCCESS;
23382350
}
@@ -2404,14 +2416,8 @@ static pj_status_t op_remove_port(pjmedia_conf *conf,
24042416
}
24052417

24062418
pj_assert( !is_port_connected( conf_port ) );
2407-
/* Remove the port. */
2408-
//pj_mutex_lock(conf->mutex);
2409-
conf->ports[port] = NULL;
2410-
//pj_mutex_unlock(conf->mutex);
2411-
2412-
if (!conf_port->is_new)
2413-
--conf->port_cnt;
24142419

2420+
/* Remove from active_listener array if needed */
24152421
if (conf_port->is_active_listener) {
24162422
pj_uint32_t idx;
24172423
pj_assert(conf->upper_bound_reg);
@@ -2425,22 +2431,55 @@ static pj_status_t op_remove_port(pjmedia_conf *conf,
24252431
}
24262432
}
24272433

2428-
PJ_LOG(4, (THIS_FILE, "Removed port %d (%.*s), port count=%d",
2429-
port, (int)conf_port->name.slen, conf_port->name.ptr,
2430-
conf->port_cnt));
2434+
PJ_LOG(4, (THIS_FILE, "Removing port %d (%.*s)",
2435+
port, (int)conf_port->name.slen, conf_port->name.ptr));
2436+
2437+
return PJ_SUCCESS;
2438+
}
2439+
2440+
2441+
/*
2442+
* Free the conf slot after port removal. This is called after the
2443+
* removal callback to ensure port IDs remain unique.
2444+
*/
2445+
static void op_remove_port2(pjmedia_conf *conf,
2446+
const pjmedia_conf_op_param *prm)
2447+
{
2448+
unsigned port = prm->remove_port.port;
2449+
struct conf_port *conf_port;
2450+
2451+
//pj_mutex_lock(conf->mutex);
2452+
2453+
conf_port = conf->ports[port];
2454+
if (conf_port == NULL) {
2455+
/* Already freed, perhaps by concurrent operation */
2456+
//pj_mutex_unlock(conf->mutex);
2457+
PJ_LOG(4,(THIS_FILE,"Port %d already freed", port));
2458+
return;
2459+
}
2460+
2461+
/* Remove the port. */
2462+
conf->ports[port] = NULL;
2463+
2464+
/* Update port count */
2465+
if (!conf_port->is_new)
2466+
--conf->port_cnt;
24312467

24322468
pj_assert(conf->port_cnt >= conf->upper_bound_reg);
24332469

2470+
//pj_mutex_unlock(conf->mutex);
2471+
2472+
PJ_LOG(4, (THIS_FILE, "Removed port %d, port count=%d",
2473+
port, conf->port_cnt));
2474+
24342475
/* Return conf_port slot to unused slots cache. */
24352476
conf_release_port( conf, port );
24362477

2437-
/* Decrease conf port ref count */
2478+
/* Decrease conf port ref count and destroy */
24382479
if (conf_port->port && conf_port->port->grp_lock)
24392480
pj_grp_lock_dec_ref(conf_port->port->grp_lock);
24402481
else
24412482
destroy_conf_port(conf_port);
2442-
2443-
return PJ_SUCCESS;
24442483
}
24452484

24462485
static void destroy_conf_port( struct conf_port *conf_port )

‎pjmedia/src/pjmedia/conference.c‎

Lines changed: 45 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,8 @@ static pj_status_t op_add_port(pjmedia_conf *conf,
292292
const pjmedia_conf_op_param *prm);
293293
static pj_status_t op_remove_port(pjmedia_conf *conf,
294294
const pjmedia_conf_op_param *prm);
295+
static void op_remove_port2(pjmedia_conf *conf,
296+
const pjmedia_conf_op_param *prm);
295297
static pj_status_t op_connect_ports(pjmedia_conf *conf,
296298
const pjmedia_conf_op_param *prm);
297299
static pj_status_t op_disconnect_ports(pjmedia_conf *conf,
@@ -373,6 +375,11 @@ static void handle_op_queue(pjmedia_conf *conf)
373375
(*conf->cb)(&info);
374376
pj_log_pop_indent();
375377
}
378+
379+
/* Free the conf slot after callback for remove port operation */
380+
if (type == PJMEDIA_CONF_OP_REMOVE_PORT) {
381+
op_remove_port2(conf, &param);
382+
}
376383
}
377384
}
378385

@@ -920,6 +927,8 @@ PJ_DEF(pj_status_t) pjmedia_conf_destroy( pjmedia_conf *conf )
920927
(*conf->cb)(&op_info);
921928
pj_log_pop_indent();
922929
}
930+
/* Free the conf slot after callback */
931+
op_remove_port2(conf, &oprm);
923932
}
924933
}
925934

@@ -1834,6 +1843,9 @@ PJ_DEF(pj_status_t) pjmedia_conf_remove_port( pjmedia_conf *conf,
18341843
pj_log_pop_indent();
18351844
}
18361845

1846+
/* Free the conf slot after callback */
1847+
op_remove_port2(conf, &prm);
1848+
18371849
pj_log_pop_indent();
18381850
return PJ_SUCCESS;
18391851
}
@@ -1926,25 +1938,50 @@ static pj_status_t op_remove_port(pjmedia_conf *conf,
19261938
conf_port->port = NULL;
19271939
}
19281940

1929-
/* Remove the port. */
1941+
PJ_LOG(4,(THIS_FILE,"Removing port %d (%.*s)",
1942+
port, (int)conf_port->name.slen, conf_port->name.ptr));
1943+
1944+
return PJ_SUCCESS;
1945+
}
1946+
1947+
1948+
/*
1949+
* Free the conf slot after port removal. This is called after the
1950+
* removal callback to ensure port IDs remain unique.
1951+
*/
1952+
static void op_remove_port2(pjmedia_conf *conf,
1953+
const pjmedia_conf_op_param *prm)
1954+
{
1955+
unsigned port = prm->remove_port.port;
1956+
struct conf_port *conf_port;
1957+
19301958
pj_mutex_lock(conf->mutex);
1959+
1960+
conf_port = conf->ports[port];
1961+
if (conf_port == NULL) {
1962+
/* Already freed, perhaps by concurrent operation */
1963+
pj_mutex_unlock(conf->mutex);
1964+
PJ_LOG(4,(THIS_FILE,"Port %d already freed", port));
1965+
return;
1966+
}
1967+
1968+
/* Free the slot */
19311969
conf->ports[port] = NULL;
1932-
pj_mutex_unlock(conf->mutex);
19331970

1971+
/* Update port count */
19341972
if (!conf_port->is_new)
19351973
--conf->port_cnt;
19361974

1937-
PJ_LOG(4,(THIS_FILE,"Removed port %d (%.*s), port count=%d",
1938-
port, (int)conf_port->name.slen, conf_port->name.ptr,
1939-
conf->port_cnt));
1975+
pj_mutex_unlock(conf->mutex);
1976+
1977+
PJ_LOG(4,(THIS_FILE,"Removed port %d, port count=%d",
1978+
port, conf->port_cnt));
19401979

1941-
/* Decrease conf port ref count */
1980+
/* Decrease conf port ref count and destroy */
19421981
if (conf_port->port && conf_port->port->grp_lock)
19431982
pj_grp_lock_dec_ref(conf_port->port->grp_lock);
19441983
else
19451984
conf_port_on_destroy(conf_port);
1946-
1947-
return PJ_SUCCESS;
19481985
}
19491986

19501987

‎pjmedia/src/pjmedia/vid_conf.c‎

Lines changed: 49 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,8 @@ static pj_status_t op_add_port(pjmedia_vid_conf *vid_conf,
147147
const pjmedia_vid_conf_op_param *prm);
148148
static pj_status_t op_remove_port(pjmedia_vid_conf *conf,
149149
const pjmedia_vid_conf_op_param *prm);
150+
static void op_remove_port2(pjmedia_vid_conf *conf,
151+
const pjmedia_vid_conf_op_param *prm);
150152
static pj_status_t op_connect_ports(pjmedia_vid_conf *conf,
151153
const pjmedia_vid_conf_op_param *prm);
152154
static pj_status_t op_disconnect_ports(pjmedia_vid_conf *conf,
@@ -234,6 +236,11 @@ static void handle_op_queue(pjmedia_vid_conf *conf)
234236
pj_log_pop_indent();
235237
}
236238

239+
/* Free the conf slot after callback for remove port operation */
240+
if (type == PJMEDIA_VID_CONF_OP_REMOVE_PORT) {
241+
op_remove_port2(conf, &param);
242+
}
243+
237244
}
238245
}
239246

@@ -375,6 +382,8 @@ PJ_DEF(pj_status_t) pjmedia_vid_conf_destroy(pjmedia_vid_conf *vid_conf)
375382
(*vid_conf->cb)(&info);
376383
pj_log_pop_indent();
377384
}
385+
/* Free the conf slot after callback */
386+
op_remove_port2(vid_conf, &prm);
378387
}
379388
}
380389

@@ -755,32 +764,57 @@ static pj_status_t op_remove_port(pjmedia_vid_conf *vid_conf,
755764
}
756765
}
757766

758-
/* Remove the port. */
767+
PJ_LOG(4,(THIS_FILE,"Removing video port %d (%.*s)",
768+
slot, (int)cport->name.slen, cport->name.ptr));
769+
770+
if (AUTO_STOP_CLOCK && vid_conf->connect_cnt == 0) {
771+
/* Warning: will stuck if this is called from the clock thread */
772+
status = pjmedia_clock_stop(vid_conf->clock);
773+
if (status != PJ_SUCCESS) {
774+
PJ_PERROR(3, (THIS_FILE, status, "Failed to stop clock"));
775+
}
776+
}
777+
return PJ_SUCCESS;
778+
}
779+
780+
781+
/*
782+
* Free the conf slot after port removal. This is called after the
783+
* removal callback to ensure port IDs remain unique.
784+
*/
785+
static void op_remove_port2(pjmedia_vid_conf *vid_conf,
786+
const pjmedia_vid_conf_op_param *prm)
787+
{
788+
unsigned slot = prm->remove_port.port;
789+
vconf_port *cport;
790+
759791
pj_mutex_lock(vid_conf->mutex);
792+
793+
cport = vid_conf->ports[slot];
794+
if (cport == NULL) {
795+
/* Already freed, perhaps by concurrent operation */
796+
pj_mutex_unlock(vid_conf->mutex);
797+
PJ_LOG(4,(THIS_FILE,"Video port %d already freed", slot));
798+
return;
799+
}
800+
801+
/* Remove the port. */
760802
vid_conf->ports[slot] = NULL;
761-
pj_mutex_unlock(vid_conf->mutex);
762803

804+
/* Update port count */
763805
if (!cport->is_new)
764806
--vid_conf->port_cnt;
765807

766-
PJ_LOG(4,(THIS_FILE,"Removed video port %d (%.*s), port count=%d",
767-
slot, (int)cport->name.slen, cport->name.ptr,
768-
vid_conf->port_cnt));
808+
pj_mutex_unlock(vid_conf->mutex);
769809

770-
/* Decrease port ref count */
810+
PJ_LOG(4,(THIS_FILE,"Removed video port %d, port count=%d",
811+
slot, vid_conf->port_cnt));
812+
813+
/* Decrease port ref count and destroy */
771814
pjmedia_port_dec_ref(cport->port);
772815

773816
/* Release pool */
774817
pj_pool_safe_release(&cport->pool);
775-
776-
if (AUTO_STOP_CLOCK && vid_conf->connect_cnt == 0) {
777-
/* Warning: will stuck if this is called from the clock thread */
778-
status = pjmedia_clock_stop(vid_conf->clock);
779-
if (status != PJ_SUCCESS) {
780-
PJ_PERROR(3, (THIS_FILE, status, "Failed to stop clock"));
781-
}
782-
}
783-
return PJ_SUCCESS;
784818
}
785819

786820
/*

0 commit comments

Comments
 (0)