Possible leak tdata when receiving 200 OK response after the invite session is destroyed.

[trengginas]

Consider this scenario:

-------> INVITE [1]
<------  200 OK INVITE [2]
-------> ACK [3]
-------> BYE [4]
<------  200 OK BYE [5]
<------  200 OK INVITE [6]
-------> ACK [7]
Dialog destroyed [8]

On normal condition, tdata created to send ACK [3] will be released when INVITE session was destroyed [5].
However If a 200 OK INVITE response was received [6] before dialog is destroyed, then the tdata created to send ACK [7] will not be released.

[nanangizz]

please put some info/comment or simply a link to this PR for future ref.

btw, the issue is reproducible and the patch does fix it, right?
also, I've found another scenario that may lead to leaked tdata, will add more info & perhaps patch later.

[trengginas]

Yes, it's reproducible and the patch fix it.

[nanangizz]

Found another scenario that may lead to leaked tdata:

1. initiates a call
2. receives 200 response for the initial INVITE
3. sends ACK, but somehow it is not delivered within ~32 secs (tsx lifetime)
4. when ACK is actually sent, app does not receives sent notification which causing ACK tdata ref cnt not decremented.

This is perhaps also the scenario occurred in this report.

Initial patch here.

[trengginas]

Do you want to incorporate the patch on this PR?

…

On Mon, May 18, 2020 at 9:38 AM Nanang Izzuddin ***@***.***> wrote: Found another scenario that may lead to leaked tdata: 1. initiates a call 2. receives 200 response for the initial INVITE 3. sends ACK, but somehow it is not delivered within ~32 secs (tsx lifetime) 4. when ACK is actually sent, app does not receives sent notification which causing ACK tdata ref cnt not decremented. This is perhaps also the scenario occurred in this report <http://lists.pjsip.org/pipermail/pjsip_lists.pjsip.org/2019-September/041490.html> . Initial patch here <https://github.com/pjsip/pjproject/files/4641724/tp-send-timeout-not-notify-app.diff.txt> . — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2432 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIZK5O3GAOQKI5CXV6QYGTRSCNS5ANCNFSM4NBG7HKQ> .

[nanangizz]

Do you want to incorporate the patch on this PR?

yes, but please do some further checks first, make sure it is not harmful.

[nanangizz]

Btw, the last patch is for generic tdata, not specifically for ACK, so before merging it in, better to test that it does not cause crash/assert on other request methods, e.g: put some sleep (>32s) right after sending the request async-ly (to make sure the tcp_flush_pending_tx() is invoked and reaching into the new line code in the patch).

[sauwming]

Btw, do we need to send ACK for the 2nd 200/OK for the INVITE? Perhaps the fix should be to avoid sending one?

i.e. If the scenario is like this instead, what should happen?
-------> INVITE [1]
<------ 200 OK INVITE [2]
-------> ACK [3]
<------ 200 OK INVITE [4]

[trengginas]

Btw, do we need to send ACK for the 2nd 200/OK for the INVITE? Perhaps the fix should be to avoid sending one?

i.e. If the scenario is like this instead, what should happen?
-------> INVITE [1]
<------ 200 OK INVITE [2]
-------> ACK [3]
<------ 200 OK INVITE [4]

#1725 is to handle this scenario

[trengginas]

tcp_flush_pending_tx

Tested here sending INVITE and REGISTER, both will leak tdata without the patch.

[nanangizz]

tcp_flush_pending_tx

Tested here sending INVITE and REGISTER, both will leak tdata without the patch.

and with the patch, no leak and no crash/assert/other-bad-thing?

[trengginas]

tcp_flush_pending_tx

Tested here sending INVITE and REGISTER, both will leak tdata without the patch.

and with the patch, no leak and no crash/assert/other-bad-thing?

No issue (assert/crash) found. Pass pjsip-test also