diff --git a/pjmedia/src/pjmedia/port.c b/pjmedia/src/pjmedia/port.c index cede0d0688..2286fd0439 100644 --- a/pjmedia/src/pjmedia/port.c +++ b/pjmedia/src/pjmedia/port.c @@ -159,16 +159,23 @@ PJ_DEF(pj_status_t) pjmedia_port_init_grp_lock( pjmedia_port *port, PJ_ASSERT_RETURN(port && pool, PJ_EINVAL); PJ_ASSERT_RETURN(port->grp_lock == NULL, PJ_EEXISTS); - /* We need to be caution on ports that do not have its own pool, - * such port is likely using app's pool, so if the app destroys the port + /* We need to be extra cautious here! + * If media port is using app's pool, and the app destroys the port * and then destroys the pool immediately, it may cause crash as the port * may have not really been destroyed and may still be accessed. - * When port has a pool, it usually implements on_destroy() for releasing - * the pool, so here we check availability of on_destroy implementation. + * + * Thus, media port needs to create and manage its own pool. Since + * app MUST NOT free this pool, port typically needs to implement + * on_destroy() for releasing the pool. Alternatively, port can also + * add a group lock handler for this purpose, but since we can't check + * this, here we just check the availability of on_destroy implementation. */ if (port->on_destroy == NULL) { - PJ_LOG(2,(THIS_FILE, "Warning, media port %s is using group lock, but " - "it does not seem to have a pool.", + PJ_LOG(2,(THIS_FILE, "Warning! Port %s on_destroy() not found. To " + "avoid premature destroy, media port must " + "manage its own pool, which can only be " + "released in on_destroy() or in its grp lock " + "handler. See PR #3928 for more info.", port->info.name.ptr)); } diff --git a/pjsip/include/pjsua2/media.hpp b/pjsip/include/pjsua2/media.hpp index 0d1631908f..fa84933c99 100644 --- a/pjsip/include/pjsua2/media.hpp +++ b/pjsip/include/pjsua2/media.hpp @@ -528,7 +528,7 @@ class AudioMediaPort : public AudioMedia private: pj_pool_t *pool; - pjmedia_port port; + pjmedia_port *port; }; /** diff --git a/pjsip/src/pjsua2/media.cpp b/pjsip/src/pjsua2/media.cpp index cefa7b3716..5e558a2374 100644 --- a/pjsip/src/pjsua2/media.cpp +++ b/pjsip/src/pjsua2/media.cpp @@ -281,26 +281,50 @@ AudioMedia* AudioMedia::typecastFromMedia(Media *media) /////////////////////////////////////////////////////////////////////////////// +struct port_data { + AudioMediaPort *mport; + pj_pool_t *pool; +}; + AudioMediaPort::AudioMediaPort() -: pool(NULL) +: pool(NULL), port(NULL) { - pj_bzero(&port, sizeof(port)); } AudioMediaPort::~AudioMediaPort() { - if (pool) { - PJSUA2_CATCH_IGNORE( unregisterMediaPort() ); - pj_pool_release(pool); - pool = NULL; + PJSUA2_CATCH_IGNORE( unregisterMediaPort() ); + if (port) { + struct port_data *pdata = static_cast + (port->port_data.pdata); + + /* Make sure port no longer accesses this object in its + * get/put_frame() callback. + */ + if (port->grp_lock) { + pj_grp_lock_acquire(port->grp_lock); + pdata->mport = NULL; + pj_grp_lock_release(port->grp_lock); + } + + pjmedia_port_destroy(port); + /* We release the pool later in port.on_destroy since + * the unregistration is async and may not have completed yet. + */ } } static pj_status_t get_frame(pjmedia_port *port, pjmedia_frame *frame) { - AudioMediaPort *mport = (AudioMediaPort *) port->port_data.pdata; + struct port_data *pdata = static_cast + (port->port_data.pdata); + AudioMediaPort *mport; MediaFrame frame_; + pj_grp_lock_acquire(port->grp_lock); + if ((mport = pdata->mport) == NULL) + goto on_return; + frame_.size = (unsigned)frame->size; mport->onFrameRequested(frame_); frame->type = frame_.type; @@ -315,20 +339,42 @@ static pj_status_t get_frame(pjmedia_port *port, pjmedia_frame *frame) pj_memcpy(frame->buf, frame_.buf.data(), frame->size); #endif - +on_return: + pj_grp_lock_release(port->grp_lock); return PJ_SUCCESS; } static pj_status_t put_frame(pjmedia_port *port, pjmedia_frame *frame) { - AudioMediaPort *mport = (AudioMediaPort *) port->port_data.pdata; + struct port_data *pdata = static_cast + (port->port_data.pdata); + AudioMediaPort *mport; MediaFrame frame_; + pj_grp_lock_acquire(port->grp_lock); + if ((mport = pdata->mport) == NULL) + goto on_return; + frame_.type = frame->type; frame_.buf.assign((char *)frame->buf, ((char *)frame->buf) + frame->size); frame_.size = (unsigned)frame->size; mport->onFrameReceived(frame_); +on_return: + pj_grp_lock_release(port->grp_lock); + return PJ_SUCCESS; +} + +static pj_status_t port_on_destroy(pjmedia_port *port) +{ + struct port_data *pdata = static_cast + (port->port_data.pdata); + pj_pool_t *pool = pdata->pool; + + if (pool) { + pj_pool_release(pool); + } + return PJ_SUCCESS; } @@ -337,6 +383,8 @@ void AudioMediaPort::createPort(const string &name, MediaFormatAudio &fmt) { pj_str_t name_; pjmedia_format fmt_; + struct port_data *pdata; + pj_status_t status; if (pool) { PJSUA2_RAISE_ERROR(PJ_EEXISTS); @@ -348,18 +396,30 @@ void AudioMediaPort::createPort(const string &name, MediaFormatAudio &fmt) } /* Init port. */ - pj_bzero(&port, sizeof(port)); + port = PJ_POOL_ZALLOC_T(pool, pjmedia_port); pj_strdup2_with_null(pool, &name_, name.c_str()); fmt_ = fmt.toPj(); - pjmedia_port_info_init2(&port.info, &name_, + pjmedia_port_info_init2(&port->info, &name_, PJMEDIA_SIG_CLASS_APP ('A', 'M', 'P'), PJMEDIA_DIR_ENCODING_DECODING, &fmt_); - port.port_data.pdata = this; - port.put_frame = &put_frame; - port.get_frame = &get_frame; + pdata = PJ_POOL_ZALLOC_T(pool, struct port_data); + pdata->mport = this; + pdata->pool = pool; + port->port_data.pdata = pdata; + port->put_frame = &put_frame; + port->get_frame = &get_frame; + + /* Must implement on_destroy() and create group lock to avoid + * race and premature destroy. + */ + port->on_destroy = &port_on_destroy; + status = pjmedia_port_init_grp_lock(port, pool, NULL); + if (status != PJ_SUCCESS) { + PJSUA2_RAISE_ERROR(status); + } - registerMediaPort2(&port, pool); + registerMediaPort2(port, pool); } ///////////////////////////////////////////////////////////////////////////////