Clone sdp in sip timer to prevent modification

[sauwming]

Fixed #2475.

The patch in this PR is proposed by the creator of the issue above, @mneuhauser.

[nanangizz]

The patch looks okay, but perhaps there is a catch.

Without the patch, app modifies sdp_neg->neg_local_sdp directly (i.e: SDP returned by pjmedia_sdp_neg_get_neg_local()), so the modification will be seen by the SDP negotiator and stored in active local SDP if the SDP nego succeeds.

With the patch, app will modify the clone, so the modification won't be seen by SDP negotiator and later pjmedia_sdp_neg_get_active_local() will return the original version (unless app invokes pjmedia_sdp_neg_cancel_offer()+pjmedia_sdp_neg_modify_local_offer()). It may be fine if the modified parts are not to be negotiated (like codec priorities, media direction).

Here is an alternative approach. Currently process_answer() in pjmedia_sdp_neg_negotiate() returns offer param which is neg->neg_local_sdp, and this SDP will be assigned as active local SDP, which will be cloned as sdp_neg->neg_local_sdp in the next SDP offer (this is why the next SDP offer is corrupted). Modifying process_answer() to return a clone of neg->neg_local_sdp or passing a clone of neg->neg_local_sdp to process_answer() should also fix the problem while maintaining the behavior (i.e: SDP negotiator sees any modifications in local SDP). Note that create_answer() clones neg->neg_local_sdp first.

[mneuhauser]

I've tested the new fix in Asterisk and it worked.

I'm fairly new to the internals of pjsip, so please forgive if that is a stupid/obvious question, but what guarantees that the transmit data pool of the outgoing INVITE (i.e., the one that was used to allocate the modified SDP value) is still in-use/untouched when the SDP is cloned in process_answer()?

From an Asterisk standpoint it would not matter if the original SDP (my version of patch) or the already modified SDP (current version) is used for the >= 2. refresh INVITEs, either works.

[sauwming]

The pool being used in process_answer() is passed as a parameter, so the function caller needs to make sure that the pool is still valid. In PJSIP, the passed pool is usually call inv's pool, so it should be valid for the duration of the call.

[nanangizz]

Good question actually. Just rechecked things, currently the transaction will maintain the last transmitted data (tdata) until the transaction is shutdown/destroyed and SDP negotiation is done before the transaction is shutdown. I don't think this behaviour will be changed in the future. Alternatively, perhaps you can use inv->pool_prov, if your PJSIP module somehow can access it, which is used by SIP timer to generate SDP offer (using pjmedia_sdp_neg_send_local_offer()).

[mneuhauser]

The pool being used in process_answer() is passed as a parameter, so the function caller needs to make sure that the pool is still valid. In PJSIP, the passed pool is usually call inv's pool, so it should be valid for the duration of the call.

I'm not concerned about that pool but about the one that is used to modify the SDP in Asterisk, i.e. pjsip_tx_data->pool.

[mneuhauser]

Good question actually. Just rechecked things, currently the transaction will maintain the last transmitted data (tdata) until the transaction is shutdown/destroyed and SDP negotiation is done before the transaction is shutdown. I don't think this behaviour will be changed in the future.

For a practical check (just to be extra careful, not that I doubt your analysis), I instrumented pjsip and Asterisk with some log statements to trace the cloning and releasing of the pool in question. Although the two operations happen on different threads, the release always happened after the clone, even when a delay of up to 1 second was introduced with pj_thread_sleep() in process_answer() before the clone operation. That closes the issue for me. I will update the Asterisk change to use the new fix.

Alternatively, perhaps you can use inv->pool_prov, if your PJSIP module somehow can access it, which is used by SIP timer to generate SDP offer (using pjmedia_sdp_neg_send_local_offer()).

I thought about that too, but I do not think that is an option in Asterisk.

Thank you all for your help.