Fixed AudioMediaPort use after free

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));
     }
 

pjsip/include/pjsua2/media.hpp

@@ -528,7 +528,7 @@ class AudioMediaPort : public AudioMedia
 
 private:
     pj_pool_t *pool;
-    pjmedia_port port;
+    pjmedia_port *port;
 };
 
 /**

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<struct port_data *>
+                                  (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<struct port_data *>
+                              (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<struct port_data *>
+                              (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<struct port_data *>
+                              (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);
 }
 
 ///////////////////////////////////////////////////////////////////////////////