[Asterisk-code-review] res pjsip transport websocket: Fix use-after-free bugs. (asterisk[13])
Mark Michelson
asteriskteam at digium.com
Wed Jun 10 10:38:05 CDT 2015
Mark Michelson has submitted this change and it was merged.
Change subject: res_pjsip_transport_websocket: Fix use-after-free bugs.
......................................................................
res_pjsip_transport_websocket: Fix use-after-free bugs.
This patch fixes use-after-free bugs caught by AddressSanitizer.
1. PJSIP transport manager may decide to destroy transport on its own.
For example, when the contact registered via websocket has not renewed
its registration in time. The transport was destoyed, but the websocket
listener thread was still active until the socket closes, and then tried
to call transport_shutdown on transport that has been freed.
Also, the transport destructor accessed wstransport->rdata.tp_info.pool
right after freeing memory that contained wstransport itself.
This patch converts transport to an ao2 object, allowing it to be
refcounted, so that it is available until both websocket listener and
pjsip transport manager are finished with it.
2. The websocket listener deletes the last reference on websocket session
when the tcp connection is closed, and it gets destroyed, but
the transport manager may still use it, for example when disconnect
happens in the middle of a SIP transaction.
A new reference to websocket session has been added that is released
with the transport to prevent this.
ASTERISK-25096 #close
Reported by: Josh Kitchens
ASTERISK-24963 #close
Reported by: Badalian Vyacheslav
Change-Id: Idc0b63eb6e459c1ddfb2430127d34b3c4d8d373b
---
M res/res_pjsip_transport_websocket.c
1 file changed, 73 insertions(+), 23 deletions(-)
Approvals:
Mark Michelson: Looks good to me, approved; Verified
Joshua Colp: Looks good to me, but someone else must approve
diff --git a/res/res_pjsip_transport_websocket.c b/res/res_pjsip_transport_websocket.c
index ab8c9c3..be30468 100644
--- a/res/res_pjsip_transport_websocket.c
+++ b/res/res_pjsip_transport_websocket.c
@@ -79,6 +79,25 @@
static pj_status_t ws_destroy(pjsip_transport *transport)
{
struct ws_transport *wstransport = (struct ws_transport *)transport;
+ int fd = ast_websocket_fd(wstransport->ws_session);
+
+ if (fd > 0) {
+ ast_websocket_close(wstransport->ws_session, 1000);
+ shutdown(fd, SHUT_RDWR);
+ }
+
+ ao2_ref(wstransport, -1);
+
+ return PJ_SUCCESS;
+}
+
+static void transport_dtor(void *arg)
+{
+ struct ws_transport *wstransport = arg;
+
+ if (wstransport->ws_session) {
+ ast_websocket_unref(wstransport->ws_session);
+ }
if (wstransport->transport.ref_cnt) {
pj_atomic_destroy(wstransport->transport.ref_cnt);
@@ -88,20 +107,28 @@
pj_lock_destroy(wstransport->transport.lock);
}
- pjsip_endpt_release_pool(wstransport->transport.endpt, wstransport->transport.pool);
+ if (wstransport->transport.endpt && wstransport->transport.pool) {
+ pjsip_endpt_release_pool(wstransport->transport.endpt, wstransport->transport.pool);
+ }
if (wstransport->rdata.tp_info.pool) {
pjsip_endpt_release_pool(wstransport->transport.endpt, wstransport->rdata.tp_info.pool);
}
-
- return PJ_SUCCESS;
}
static int transport_shutdown(void *data)
{
- pjsip_transport *transport = data;
+ struct ws_transport *wstransport = data;
- pjsip_transport_shutdown(transport);
+ if (!wstransport->transport.is_shutdown && !wstransport->transport.is_destroying) {
+ pjsip_transport_shutdown(&wstransport->transport);
+ }
+
+ /* Note that the destructor calls PJSIP functions,
+ * therefore it must be called in a PJSIP thread.
+ */
+ ao2_ref(wstransport, -1);
+
return 0;
}
@@ -116,32 +143,45 @@
static int transport_create(void *data)
{
struct transport_create_data *create_data = data;
- struct ws_transport *newtransport;
+ struct ws_transport *newtransport = NULL;
pjsip_endpoint *endpt = ast_sip_get_pjsip_endpoint();
struct pjsip_tpmgr *tpmgr = pjsip_endpt_get_tpmgr(endpt);
pj_pool_t *pool;
-
pj_str_t buf;
+ pj_status_t status;
+
+ newtransport = ao2_t_alloc_options(sizeof(*newtransport), transport_dtor,
+ AO2_ALLOC_OPT_LOCK_NOLOCK, "pjsip websocket transport");
+ if (!newtransport) {
+ ast_log(LOG_ERROR, "Failed to allocate WebSocket transport.\n");
+ goto on_error;
+ }
+
+ newtransport->transport.endpt = endpt;
if (!(pool = pjsip_endpt_create_pool(endpt, "ws", 512, 512))) {
ast_log(LOG_ERROR, "Failed to allocate WebSocket endpoint pool.\n");
- return -1;
+ goto on_error;
}
-
- if (!(newtransport = PJ_POOL_ZALLOC_T(pool, struct ws_transport))) {
- ast_log(LOG_ERROR, "Failed to allocate WebSocket transport.\n");
- pjsip_endpt_release_pool(endpt, pool);
- return -1;
- }
-
- newtransport->ws_session = create_data->ws_session;
-
- pj_atomic_create(pool, 0, &newtransport->transport.ref_cnt);
- pj_lock_create_recursive_mutex(pool, pool->obj_name, &newtransport->transport.lock);
newtransport->transport.pool = pool;
+ newtransport->ws_session = create_data->ws_session;
+
+ /* Keep the session until transport dies */
+ ast_websocket_ref(newtransport->ws_session);
+
+ status = pj_atomic_create(pool, 0, &newtransport->transport.ref_cnt);
+ if (status != PJ_SUCCESS) {
+ goto on_error;
+ }
+
+ status = pj_lock_create_recursive_mutex(pool, pool->obj_name, &newtransport->transport.lock);
+ if (status != PJ_SUCCESS) {
+ goto on_error;
+ }
+
pj_sockaddr_parse(pj_AF_UNSPEC(), 0, pj_cstr(&buf, ast_sockaddr_stringify(ast_websocket_remote_address(newtransport->ws_session))), &newtransport->transport.key.rem_addr);
newtransport->transport.key.rem_addr.addr.sa_family = pj_AF_INET();
newtransport->transport.key.type = ast_websocket_is_secure(newtransport->ws_session) ? transport_type_wss : transport_type_ws;
@@ -159,24 +199,34 @@
newtransport->transport.flag = pjsip_transport_get_flag_from_type((pjsip_transport_type_e)newtransport->transport.key.type);
newtransport->transport.info = (char *)pj_pool_alloc(newtransport->transport.pool, 64);
- newtransport->transport.endpt = endpt;
newtransport->transport.tpmgr = tpmgr;
newtransport->transport.send_msg = &ws_send_msg;
newtransport->transport.destroy = &ws_destroy;
- pjsip_transport_register(newtransport->transport.tpmgr, (pjsip_transport *)newtransport);
+ status = pjsip_transport_register(newtransport->transport.tpmgr,
+ (pjsip_transport *)newtransport);
+ if (status != PJ_SUCCESS) {
+ goto on_error;
+ }
+
+ /* Add a reference for pjsip transport manager */
+ ao2_ref(newtransport, +1);
newtransport->rdata.tp_info.transport = &newtransport->transport;
newtransport->rdata.tp_info.pool = pjsip_endpt_create_pool(endpt, "rtd%p",
PJSIP_POOL_RDATA_LEN, PJSIP_POOL_RDATA_INC);
if (!newtransport->rdata.tp_info.pool) {
ast_log(LOG_ERROR, "Failed to allocate WebSocket rdata.\n");
- pjsip_endpt_release_pool(endpt, pool);
- return -1;
+ pjsip_transport_destroy((pjsip_transport *)newtransport);
+ goto on_error;
}
create_data->transport = newtransport;
return 0;
+
+on_error:
+ ao2_cleanup(newtransport);
+ return -1;
}
struct transport_read_data {
--
To view, visit https://gerrit.asterisk.org/619
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Idc0b63eb6e459c1ddfb2430127d34b3c4d8d373b
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Ivan Poddubny <ivan.poddubny at gmail.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
More information about the asterisk-code-review
mailing list