[Asterisk-code-review] res pjsip notify.c: enable in-dialog NOTIFY (asterisk[15])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8374 )

Change subject: res_pjsip_notify.c: enable in-dialog NOTIFY
......................................................................


Patch Set 7: Code-Review-1

(3 comments)

While creating the patch for the v13 branch it occurred to me that sending the NOTIFY in the dialog happened under the wrong serializer.  This is bad as it can potentially cause memory corruption.  That NOTIFY must be sent using the channel session's serializer.

https://gerrit.asterisk.org/#/c/8374/7/res/res_pjsip_notify.c
File res/res_pjsip_notify.c:

https://gerrit.asterisk.org/#/c/8374/7/res/res_pjsip_notify.c@303
PS7, Line 303: 	struct ast_channel *channel;
This should be changed to a struct ast_sip_session *session pointer.


https://gerrit.asterisk.org/#/c/8374/7/res/res_pjsip_notify.c@739
PS7, Line 739: 	dlg = ast_sip_session_get_dialog_for_channel(data->channel);
             : 	/* keep the channel locked so the dialog can't go away */
When run under the channel session's serializer the dialog won't go away after you check it's validity.  Also I don't think keeping the channel lock prevents the dialog from going away anyway.


https://gerrit.asterisk.org/#/c/8374/7/res/res_pjsip_notify.c@863
PS7, Line 863: 	if (ast_sip_push_task(NULL, notify_channel, data)) {
This should be pushing the notify_channel task onto the channel session's serializer.  This ensures that NOTIFY messages to the same channel go out in the same order if they are rapidly sent.  Also it will avoid potential threading issues.

An example that can be used is chan_pjsip.c:chan_pjsip_sendtext().  That function sends MESSAGE requests in dialog.



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

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: If7f3151a6d633e414d5dc319d5efc1443c43dd29
Gerrit-Change-Number: 8374
Gerrit-PatchSet: 7
Gerrit-Owner: Nathan Bruning <nathan at iperity.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: lvl <digium at lvlconsultancy.nl>
Gerrit-Comment-Date: Wed, 14 Mar 2018 17:16:52 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180314/d2ddba76/attachment.html>