Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tdata leak when pjsip_inv_initial_answer() return error #3741

Merged
merged 4 commits into from
Oct 19, 2023
Merged

Fix tdata leak when pjsip_inv_initial_answer() return error #3741

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43dd94f
Fix tdata leak when pjsip_inv_initial_answer() return error
trengginas Oct 17, 2023
d013d26
Set the inv->last_answer with tdata
trengginas Oct 18, 2023
03c8a8a
Modification based on comments
trengginas Oct 19, 2023
c979e50
Modify doc
trengginas Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pjsip/src/pjsip-ua/sip_inv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2664,8 +2664,8 @@ PJ_DEF(pj_status_t) pjsip_inv_initial_answer( pjsip_inv_session *inv,
pj_status_t status2;

status2 = pjsip_dlg_modify_response(inv->dlg, tdata, st_code2, NULL);
pjsip_tx_data_dec_ref(tdata);
if (status2 != PJ_SUCCESS) {
pjsip_tx_data_dec_ref(tdata);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

goto on_return;
}
status2 = pjsip_timer_update_resp(inv, tdata);
Expand Down
3 changes: 3 additions & 0 deletions pjsip/src/pjsua-lib/pjsua_call.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5552,6 +5552,9 @@ static void pjsua_call_on_rx_offer(pjsip_inv_session *inv,
(pjsip_rx_data *)param->rdata,
100, NULL, NULL, &response);
if (status != PJ_SUCCESS) {
if (response) {
pjsip_tx_data_dec_ref(response);
}
PJ_PERROR(3, (THIS_FILE, status,
"Failed to create initial answer"));
goto on_return;
Expand Down
Loading