This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
dexter gerrit-no-reply at lists.osmocom.orgHello Harald Welte, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/6006 to look at the new patch set (#6). 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_endp.h M include/osmocom/mgcp/mgcp_internal.h M src/libosmo-mgcp/mgcp_conn.c M src/libosmo-mgcp/mgcp_endp.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/6 diff --git a/include/osmocom/mgcp/mgcp_endp.h b/include/osmocom/mgcp/mgcp_endp.h index 0225879..a486dcd 100644 --- a/include/osmocom/mgcp/mgcp_endp.h +++ b/include/osmocom/mgcp/mgcp_endp.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 b/include/osmocom/mgcp/mgcp_internal.h index 9c57e3f..e4009e8 100644 --- a/include/osmocom/mgcp/mgcp_internal.h +++ b/include/osmocom/mgcp/mgcp_internal.h @@ -265,6 +265,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 *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 cc115a9..62cbdba 100644 --- a/src/libosmo-mgcp/mgcp_conn.c +++ b/src/libosmo-mgcp/mgcp_conn.c @@ -201,6 +201,12 @@ if (!conn) return; + /* 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: osmux_disable_conn(&conn->u.rtp); diff --git a/src/libosmo-mgcp/mgcp_endp.c b/src/libosmo-mgcp/mgcp_endp.c index 6d3a6d4..fa2dd28 100644 --- a/src/libosmo-mgcp/mgcp_endp.c +++ b/src/libosmo-mgcp/mgcp_endp.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 ef6357b..6923b97 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 *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: 6 Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Owner: dexter <pmaier at sysmocom.de> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pmaier at sysmocom.de>