Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit


to look at the new patch set (#5).

mgcp: fix use-after-free and add callback for endpoint cleanup

Since we will support multiple different types of endpoints in the
future, all these endpoints will handle connections slightly different
and there will be possibly state that needs to be kept consistant
when a connection is deleted.

In mgcp_network.c where we implement the callback that is used to
create an rtp-bride-endpoint. In that callback we cache the pointer
of the connection we where we want to bride to (opposite connection).
When one of the connections is deleted using a DLCX operation, the
pointer is still there and the next incoming packet causes a use-
after-free segfault.

- introduce an endpoint specific callback function that is executed
  before removing the connection.

- implement the endpoint specific callback for rtp bridge endpoints,
  so that the use-after-free is prevented.

Change-Id: I921d9bbe58be1c3298e164a37f3c974880b3759f
M include/osmocom/mgcp/mgcp_ep.h
M include/osmocom/mgcp/mgcp_internal.h
M src/libosmo-mgcp/mgcp_conn.c
M src/libosmo-mgcp/mgcp_ep.c
M src/libosmo-mgcp/mgcp_network.c
5 files changed, 38 insertions(+), 1 deletion(-)

  git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/06/6006/5

diff --git a/include/osmocom/mgcp/mgcp_ep.h b/include/osmocom/mgcp/mgcp_ep.h
index 915f542..7b4ccee 100644
--- a/include/osmocom/mgcp/mgcp_ep.h
+++ b/include/osmocom/mgcp/mgcp_ep.h
@@ -33,6 +33,13 @@
                                     char *buf, unsigned int buf_size,
                                     struct mgcp_conn *conn);
+/* Callback type for endpoint specific cleanup actions. This function
+ * is automatically executed when a connection is freed (see mgcp_conn_free()
+ * in mgcp_conn.c). Depending on the type of the endpoint there may be endpoint
+ * specific things to take care of once a connection has been removed. */
+typedef void (*mgcp_cleanup_cp) (struct mgcp_endpoint *endp,
+                                struct mgcp_conn *conn);
 /*! MGCP endpoint properties */
 struct mgcp_endpoint_type {
        /*!< maximum number of connections */
@@ -40,6 +47,9 @@
        /*!< callback that defines how to dispatch incoming RTP data */
        mgcp_dispatch_rtp_cb dispatch_rtp_cb;
+       /*!< callback that implements endpoint specific cleanup actions */
+       mgcp_cleanup_cp cleanup_cb;
 /*! MGCP endpoint typeset */
diff --git a/include/osmocom/mgcp/mgcp_internal.h 
index c0ee556..5de28ee 100644
--- a/include/osmocom/mgcp/mgcp_internal.h
+++ b/include/osmocom/mgcp/mgcp_internal.h
@@ -285,6 +285,7 @@
 int mgcp_send_dummy(struct mgcp_endpoint *endp, struct mgcp_conn_rtp *conn);
 int mgcp_dispatch_rtp_bridge_cb(int proto, struct sockaddr_in *addr, char *buf,
                                unsigned int buf_size, struct mgcp_conn *conn);
+void mgcp_cleanup_rtp_bridge_cb(struct mgcp_endpoint *endp, struct mgcp_conn 
 int mgcp_bind_net_rtp_port(struct mgcp_endpoint *endp, int rtp_port,
                           struct mgcp_conn_rtp *conn);
 void mgcp_free_rtp_port(struct mgcp_rtp_end *end);
diff --git a/src/libosmo-mgcp/mgcp_conn.c b/src/libosmo-mgcp/mgcp_conn.c
index a136fda..33c4165 100644
--- a/src/libosmo-mgcp/mgcp_conn.c
+++ b/src/libosmo-mgcp/mgcp_conn.c
@@ -201,6 +201,12 @@
        if (!conn)
+       /* Run endpoint cleanup action. By this we inform the endpoint about
+        * the removal of the connection and allow it to clean up its inner
+        * state accordingly */
+       if (endp->type->cleanup_cb)
+               endp->type->cleanup_cb(endp, conn);
        switch (conn->type) {
        case MGCP_CONN_TYPE_RTP:
diff --git a/src/libosmo-mgcp/mgcp_ep.c b/src/libosmo-mgcp/mgcp_ep.c
index 68e36da..e23d0f1 100644
--- a/src/libosmo-mgcp/mgcp_ep.c
+++ b/src/libosmo-mgcp/mgcp_ep.c
@@ -28,7 +28,8 @@
 const struct mgcp_endpoint_typeset ep_typeset = {
        /* Specify endpoint properties for RTP endpoint */
        .rtp.max_conns = 2,
-       .rtp.dispatch_rtp_cb = mgcp_dispatch_rtp_bridge_cb
+       .rtp.dispatch_rtp_cb = mgcp_dispatch_rtp_bridge_cb,
+       .rtp.cleanup_cb = mgcp_cleanup_rtp_bridge_cb
 /*! release endpoint, all open connections are closed.
diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c
index 6cecbb0..4e22e63 100644
--- a/src/libosmo-mgcp/mgcp_network.c
+++ b/src/libosmo-mgcp/mgcp_network.c
@@ -1043,6 +1043,25 @@
+/*! cleanup an endpoint when a connection on an RTP bridge endpoint is removed.
+ *  \param[in] endp Endpoint on which the connection resides.
+ *  \param[in] conn Connection that is about to be removed (ignored).
+ *  \returns 0 on success, -1 on ERROR. */
+void mgcp_cleanup_rtp_bridge_cb(struct mgcp_endpoint *endp, struct mgcp_conn 
+       struct mgcp_conn *conn_cleanup;
+       /* In mgcp_dispatch_rtp_bridge_cb() we use conn->priv to cache the
+        * pointer to the destination connection, so that we do not have
+        * to go through the list every time an RTP packet arrives. To prevent
+        * a use-after-free situation we invalidate this information for all
+        * connections present when one connection is removed from the
+        * endpoint. */
+       llist_for_each_entry(conn_cleanup, &endp->conns, entry) {
+               conn_cleanup->priv = NULL;
+       }
 /* Handle incoming RTP data from NET */
 static int rtp_data_net(struct osmo_fd *fd, unsigned int what)

To view, visit https://gerrit.osmocom.org/6006
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I921d9bbe58be1c3298e164a37f3c974880b3759f
Gerrit-PatchSet: 5
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: dexter <pma...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pma...@sysmocom.de>

Reply via email to