[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.

Fraid not.  unless both the locking and the serialization are in, I get the SEGV.

-- 
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