[asterisk-dev] [Code Review] 3780: res_pjsip_outbound_publish / res_pjsip_publish_asterisk: Add outbound PUBLISH support with 'asterisk' event type.

Mon Aug 4 14:00:24 CDT 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3780/#review12969
-----------------------------------------------------------

There appears to be the potential for some refcounting badness with the ast_sip_outbound_publish structure. The problem is that the structure is passed as the "token" parameter to pjsip_publishc_create() without adjusting its refcount. Essentially, PJSIP's publish client and sorcery are sharing the same reference to the structure. This means that if someone were to perform a reload, and sorcery were to drop its reference to the structure, this could result in turr'ble things happening if that reload were to occur in the middle of a PUBLISH transaction. Consider the following:

* We send a PUBLISH
* We receive a response to the PUBLISH. sip_outbound_publish_callback() is called.
* Before the callback completes, sorcery is reloaded, sorcery drops its ref to the ast_sip_outbound_publish.
* Sorcery dropping its ref results in the ast_sip_outbound_publish() being destroyed.
* Now the callback continues, and attempts to do something like ao2_unlock(publish->state)
* Kablooey

Ideally, a way you could implement things safely would be to employ an algorithm like the following:
1. When creating the publish client, bump the refcount of the ast_sip_outbound_publish so the client has its own reference.
2. When sorcery removes its reference to the object, set a flag on it, and use pjsip_publishc_unpublish() to send a final PUBLISH.
3. When sip_outbound_publish_callback() is called for your final PUBLISH, remove the publish client's reference to the object. This will result in the object's destruction.

This way, you will only destroy the object after all SIP traffic having to do with the object has ceased. The trickiest part of implementing this is step 2. When an object is removed from configuration and sorcery is reloaded, a sorcery observer's delete() callback is not called. However, sorcery observers do get notified whenever an object type gets loaded/reloaded. So you could employ a sorcery observer that is called into when objects of type "outbound-publish" are reloaded. When the object type is reloaded, you can see if any known outbound publications have been removed from sorcery. If so, perform the rest of step 2 on the removed objects.

/trunk/include/asterisk/res_pjsip_outbound_publish.h
<https://reviewboard.asterisk.org/r/3780/#comment23355>

    "using" is repeated.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23357>

    "subtype" is not a documented configOption.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23356>

    s/registrations/publications/

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23358>

    s/Request/request/

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23361>

    You can eliminate the dynamic memory allocation by using ast_uuid_generate_str()

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23362>

    You'll need to add a call to pjsip_tx_data_dec_ref(tdata) on this failure path.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23365>

    There are other areas in the code that follow a pattern like this one. And in those, we typically don't care too much if the far end continues to reply with a 401/407 to our repeated requests. However, in this case, if a PUBLISH repeatedly is answered with a 401/407, this results in the queue not getting serviced. There should probably be something in place here to ensure that we bail on a publication if we aren't able to authenticate.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23371>

    After destroying publish->state->client, set it to NULL and call sip_outbound_publish_alloc() here. The current approach results in the outbound_proxy setting being "lost" when the new pjsip_publishc is created/initialized.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23369>

    This return path leaves publish->state->client in an unknown state and will likely cause a crash the next time the queue is serviced. A better solution here is to kill off the client completely here.

    Note that "killing the client completely" is something that currently can't really be done since sorcery is the arbiter of the lifetime of the publication object. However, you could potentially destroy publish->state and set it NULL. This could be used as an indication of a "dead" publication that can't have messages added to its queue due to some unrecoverable error.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23370>

    This is another case where returning without performing some client destruction will lead to issues.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23368>

    The simple return in this failure case  leaves the queue unserviced until a new publication is queued. It's also likely that future publications will result in 423 responses since we have not attempted to lengthen our expiration time any.

    I have a couple of suggestions:
    1) Be friendlier in the face of non-compliance. If a UA is sending 423 and not giving you any help, you can try something like doubling your expiration and trying again. If the expiration gets beyond some maximum, you can claim that it's a lost cause to try to publish to this UA and kill the client.
    2) Take a 423 with no Min-Expires header as a fatal failure condition. If they claim the expiration is too brief but provide no hint as to how to comply, then kill the client completely.

/trunk/res/res_pjsip_outbound_publish.c
<https://reviewboard.asterisk.org/r/3780/#comment23366>

    s/402/423/

- Mark Michelson

On Aug. 3, 2014, 10:58 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3780/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2014, 10:58 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This adds two PJSIP modules which add outbound PUBLISH support and an 'asterisk' event type.
> 
> The res_pjsip_outbound_publish module is a common module which provides basic logic for setting up outbound PUBLISH clients, handling authentication requests, handling configuration, and lifetime. Extra modules implement specific event types which are registered with res_pjsip_outbound_publish. Since it takes care of configuration when an outbound PUBLISH is configured extra configuration can be passed to the event type implementation to further configure itself.
> 
> The res_pjsip_publish_asterisk module implements inbound and outbound support for an 'asterisk' event type. This event type conveys device and mailbox state between Asterisk instances using a JSON content body. As internal device or mailbox state changes the module sends a PUBLISH message to other configured instances. When a PUBLISH is received the contents are examined and a device or mailbox state change queued up within Asterisk. To restrict what is sent and received filtering is available using regular expressions which can reduce SIP traffic.
> 
> A wiki page is available at https://wiki.asterisk.org/wiki/display/~jcolp/Exchanging+Device+and+Mailbox+State+Using+PJSIP which has some configuration details with some examples. This should also be reviewed.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_pjsip_pubsub.exports.in 419936 
>   /trunk/res/res_pjsip_pubsub.c 419936 
>   /trunk/res/res_pjsip_publish_asterisk.c PRE-CREATION 
>   /trunk/res/res_pjsip_outbound_publish.exports.in PRE-CREATION 
>   /trunk/res/res_pjsip_outbound_publish.c PRE-CREATION 
>   /trunk/include/asterisk/res_pjsip_pubsub.h 419936 
>   /trunk/include/asterisk/res_pjsip_outbound_publish.h PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/3780/diff/
> 
> 
> Testing
> -------
> 
> Set up two Asterisk instances, configured both sides to publish to eachother, made calls and manipulated voicemail. Watched PUBLISH messages go between them and state change.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140804/f0356e0b/attachment-0001.html>