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

[Joshua Colp]

Joshua Colp has posted comments on this change.

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


Patch Set 5: Code-Review-1

(3 comments)

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

Line 123: 	ao2_ref(wstransport, -1);
I don't think this should be dropped here, as it took me looking at the entire file and following the flow to make sure the transport was released. I think it makes more sense to drop it at the end of websocket_cb instead. This function is really about crossing the thread boundary to invoke the PJSIP transport shutdown.


Line 192: 	pjsip_transport_register(newtransport->transport.tpmgr, (pjsip_transport *)newtransport);
This currently steals the initial reference to newtransport, which makes reference counting hard to follow. Do an explicit ao2_ref here and add a comment saying that the transport manager needs a reference. The initial reference should be held by the function which is asking this to be created. In the end the count becomes the same, but it's just logically easier to follow.


Line 314: 	ao2_ref(transport, +1);
Don't do a ref here (which was needed as before you stole it) - instead use the ref that exists as a result of the transport creation.


-- 
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: 5
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-HasComments: Yes