[Asterisk-code-review] res pjsip transport websocket: Fix use-after-free bugs. (asterisk[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: res_pjsip_transport_websocket: Fix use-after-free bugs.
......................................................................


Patch Set 6:

(2 comments)

https://gerrit.asterisk.org/#/c/598/6/res/res_pjsip_transport_websocket.c
File res/res_pjsip_transport_websocket.c:

Line 108: 	pjsip_endpt_release_pool(wstransport->transport.endpt, wstransport->transport.pool);
This needs a NULL check for the transport's pool since if wstransport->transport.pool is null this call will crash.

Also, probably doesn't hurt to check the 'endpt'. It doesn't seem to be currently used by release_pool, but older versions may have used it at one time.


Line 205: 	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_transport_destroy((pjsip_transport *)newtransport);
        : 		return -1;
I think this off nominal path needs to also 'goto on_error' since the ref gets bumped for the pjsip transport manager, which gets decremented by transport destroy, but the original allocation ref does not get decremented thus causing a ref leak.


-- 
To view, visit https://gerrit.asterisk.org/598
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idc0b63eb6e459c1ddfb2430127d34b3c4d8d373b
Gerrit-PatchSet: 6
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Ivan Poddubny <ivan.poddubny at gmail.com>
Gerrit-Reviewer: Ivan Poddubny <ivan.poddubny at gmail.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-HasComments: Yes