Fix tdata leak when pjsip_inv_initial_answer() return error

[trengginas]

When pjsip_inv_initial_answer() return error, it might set the tdata to be sent.
This will lead to leaked tdata if the caller doesn't manually destroy it (e.g: calling pjsip_tx_data_dec_ref() or sending the tdata using pjsip_inv_send_msg()).
The issues related to this:

1. The call to pjsip_dlg_modify_response() in

   pjproject/pjsip/src/pjsip-ua/sip_inv.c

   Line 2666 in 43dd94f

   status2 = pjsip_dlg_modify_response(inv->dlg, tdata, st_code2, NULL);

   
   will add the reference to the returned tdata. This will lead to a leak.
2. In the case of receiving new offer on the re-INVTE. The tdata returned from pjsip_inv_initial_answer() is not checked and lead to a leak.
   

   pjproject/pjsip/src/pjsua-lib/pjsua_call.c

   Line 5551 in 43dd94f

   status = pjsip_inv_initial_answer(inv,

[nanangizz]

Here the ref count is not decremented when not-success, but below after pjsip_timer_update_resp it is decremented. Is it okay?

Also, please make sure that the pattern is consistent for every pjsip_dlg_modify_response in the lib.

[trengginas]

The issue is that the tdata->ref_cnt will be 2 when pjsip_inv_initial_answer() returns.
e.g: the pjsip_tx_data_dec_ref() after pjsip_timer_update_resp() will not release the tdata.
I think setting the tdata->last_answer would be better to avoid the inconsistency and let inv/tsx decrement it later.

[nanangizz]

• Setting inv->last_answer is fine only when p_tdata is set (so app will send it or dec ref it), otherwise it will still leak?
• pjsip_dlg_modify_response does not mention about add ref count in its spec/doc, I think it's better to add it now.
• Also from a quick check, in some other places, e.g: when process_answer() fails, it seems that the tdata ref count is not properly decremented? (need test all affected scenarios to make sure things are fine).

[trengginas]

• At the moment we are missing a dec ref due to pjsip_dlg_modify_response. So, it will still leak even if p_tdata is not set. We can set inv->last_answer only if p_tdata is set, however we need to call an additional pjsip_tx_data_dec_ref in the case p_tdata is not set.
• Right, will add the info to the doc
• You are right, we need to check the code after pjsip_dlg_modify_response. We need to an additional pjsip_tx_data_dec_ref
  e.g:
  

  pjproject/pjsip/src/pjsip-ua/sip_inv.c

  Line 2758 in 43dd94f

  pjsip_tx_data_dec_ref(last_res);

[sauwming]

I am actually a bit puzzled as to why we need to add tdata ref in pjsip_dlg_modify_response(). Is there a way for us to avoid adding the ref?

• Adding the reference in a function called 'modify' doesn't seem very intuitive, and rather unexpected. Even we did not know this before (and especially so because the doc never says anything about it).
• It also seems "ugly" if we have to decrement the ref twice on some failure cases.

From the code, it states: "Must add reference counter, since tsx_send_msg() will decrement it." And it will probably causes crash if we remove it, but perhaps there's another way?

[nanangizz]

Well, the API has been there since the beginning, see here, so I think modifying the behavior will cause a major backward compatibility issue.

We can always introduce an alternative API (without add ref) and replace all usages of the old one in the library, but perhaps too much hassle and we may still need to maintain the old one for quite some time for existing apps.

[sauwming]

Yes, if it introduces backward incompatibility, I agree that we shouldn't change it. Also, we don't need to create additional API since it's not a major issue.

I was just wondering why we need the additional tdata ref increment inside modify(), and making the tdata ref to become two, while if we use create(), it seems that the tdata ref is only one before we send it?

[nanangizz]

Just a guess. Perhaps because modify_response() is designated to work with a saved tdata such as inv->last_answer or tsx->last_tx, so the saved tdata will not be destroyed after being sent, e.g:

tdata = inv->last_answer;
modify_response(tdata);
send_response(tdata);
/* inv->last_answer will still be maintained here */

[nanangizz]

• while here, perhaps add all params docs too?
• adding a bit background about why p_tdata is set upon failure sounds nice? For example When this function returning non-PJ_SUCCESS, it may be caused by an unacceptable INVITE request. In such cases the function will generate an appropriate answer in p_tdata, e.g: when session timer header Session-Expires is too low, the generated answer will include Min-SE header. If the generated answer is not sent, it must be destroyed, i.e: using pjsip_tx_data_dec_ref(), to avoid resource leak.