[Asterisk-code-review] res pjsip pubsub: Add more locking and checking around NOTIFY (asterisk[13])

[George Joseph]

George Joseph has posted comments on this change.

Change subject: res_pjsip_pubsub: Add more locking and checking around NOTIFY
......................................................................


Patch Set 4:

> Some of the changes presented in this review are not necessary.
 > 
 > First, I double-checked and the dialog is actually locked on all
 > calls into pubsub_on_evsub_state(). There are four ways that the
 > evsub code can call into us:
 > * On subscription timeout
 > * On direct subscription termination
 > * When sending a request
 > * When transaction state changes
 > 
 > For the first three, the evsub code locks the dialog directly
 > before calling into us. For the fourth, it's a bit harder to trace,
 > but the dialog is actually locked before reaching the evsub code.
 > Here's an example of a backtrace I took:
 > 
 > #0: [0x7fbc8671a5ee] res/res_pjsip_pubsub.c:3404 pubsub_on_evsub_state()
 > #1: [0x7fbcac20f988] evsub.c:0 set_state()
 > #2: [0x7fbcac210ff9] evsub.c:0 mod_evsub_on_tsx_state()
 > #3: [0x7fbc87be1fa5] :0 pjsip_dlg_on_tsx_state() (0x7fbc87be1f30+75)
 > #4: [0x7fbc87bdcd79] sip_transaction.c:0 tsx_set_state()
 > #5: [0x7fbc87bdda5e] sip_transaction.c:0 tsx_on_state_null()
 > #6: [0x7fbc87bdf95f] :0 pjsip_tsx_recv_msg() (0x7fbc87bdf8e0+7F)
 > #7: [0x7fbc87be1cbf] :0 pjsip_dlg_on_rx_request() (0x7fbc87be1bb0+10F)
 > #8: [0x7fbc87be32e3] sip_ua_layer.c:0 mod_ua_on_rx_request()
 > #9: [0x7fbc87bcac87] :0 pjsip_endpt_process_rx_data()
 > (0x7fbc87bcab40+147)
 > #10: [0x7fbcac4436c9] res_pjsip/pjsip_distributor.c:766
 > distribute()
 > #11: [0x5f5ac1] main/taskprocessor.c:940 ast_taskprocessor_execute()
 > (0x5f59b4+10D)
 > #12: [0x5ff259] main/threadpool.c:1322 execute_tasks()
 > #13: [0x5f5ac1] main/taskprocessor.c:940 ast_taskprocessor_execute()
 > (0x5f59b4+10D)
 > #14: [0x5fd513] main/threadpool.c:351 threadpool_execute()
 > #15: [0x5feb55] main/threadpool.c:1105 worker_active()
 > #16: [0x5fe90e] main/threadpool.c:1025 worker_start()
 > #17: [0x60b128] main/utils.c:1235 dummy_start()
 > 
 > The dialog is locked in frame #3. The takeaway here is that the
 > transaction user is the dialog layer, not evsub. The transaction
 > layer calls into the dialog layer directly on transaction state
 > changes. The dialog layer then passes the transaction state change
 > to its dialog usages. All transaction state changes (including
 > transaction timeout) will lock the dialog because of this
 > structuring.
 > 
 > So with all four paths covered, the dialog will be locked in all
 > cases before being called into by the evsub code.
 > 
 > 
 > The other change that is not necessary is the added serialization
 > in ast_sip_subscription_notify(). Currently, there are two callers
 > of this function, one in res_pjsip_mwi.c and one in
 > res_pjsip_exten_state.c.
 > 
 > The res_pjsip_exten_state.c usage is straightforward. You can see
 > that it pushes a notify_task() onto the subscription's serializer.
 > That notify_task() is what then calls ast_sip_subscription_notify().
 > 
 > The res_pjsip_mwi.c usage is a bit less easy to follow. The reason
 > is because there exist both solicited and unsolicited NOTIFYs for
 > MWI. Solicited MWIs have a subscription associated with them and
 > therefore push a serialized_notify() onto the subscription
 > serializer. ast_sip_subscription_notify() is then called from the
 > subscription serializer. For unsolicited MWI notifications, there
 > is no corresponding subscription object. Therefore, the
 > serialized_notify() is pushed to an arbitrary serializer.
 > Furthermore, because the NOTIFY is unsolicited, we don't even call
 > ast_sip_subscription_notify() on this path. We instead just send an
 > out-of-dialog request to the endpoint.

Your arguments are irrefutable...except by the empirical evidence. :)

If I remove the locking in pubsub_on_evsub_state, I get the SEGV within 30 seconds of starting the test.

I'm retesting with and without the serialization in ast_sip_subscription_notify.

-- 
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: 4
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: No