[Asterisk-code-review] res pjsip pubsub: Address SEGV when attempting to terminate ... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: res_pjsip_pubsub: Address SEGV when attempting to terminate a subscription
......................................................................


Patch Set 6: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/3018/6/res/res_pjsip_pubsub.c
File res/res_pjsip_pubsub.c:

Line 2332: 	sub_tree->send_scheduled_notify = 1;
Pre-existing bug pointed out in review of previous patch attempt:

Say the user set the sub_tree->notification_batch_interval to 1 ms.  It is possible for sched_cb() to execute before we then set sub_tree->send_scheduled_notify.  It is also possible for the then serialized serialized_send_notify() to execute before we set sub_tree->send_scheduled_notify.  In this case serialized_send_notify() will do nothing because we haven't set the flag yet.

We should set the flag before scheduling sched_cb() and for completeness clear it if scheduling fails.  We don't really need to clear it if it fails because it is only acted on by serialized_send_notify().


Line 2344: 	if (!sub->tree->evsub || is_evsub_destroyed(sub->tree->evsub) || sub->tree->pending_terminates) {
long line


Line 2358: 		sub->tree->pending_terminates++;
Is it really possible to have more than one termination pending?

We are checking for zero before we increment it everywhere.  We are explicitly locking the dialog everywhere except in pubsub_on_evsub_state() and pubsub_on_server_timeout() which supposedly already have the dialog locked.  If the dialog isn't locked before the two explicit exceptions then the increment should be considered unsafe because the read for the zero test and the read increment write operation code is not thread safe.

pending_terminates should thus be a boolean, renamed to pending_terminate, and only set to 0 or 1.


PS6, Line 3362: 	if (sub_tree->pending_terminates > 1) {
              : 		sub_tree->pending_terminates--;
              : 		pjsip_dlg_dec_lock(dlg);
              : 		ao2_cleanup(sub_tree);
              : 		return 0;
              : 	}
This should be deleted since pending_terminates can never become more than one.


PS6, Line 3444: 	} else if (sub_tree->pending_terminates > 1) {
              : 		return;
Cannot happen since pending_terminates cannot be more than one.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I779d11802cf672a51392e62a74a1216596075ba1
Gerrit-PatchSet: 6
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes