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.

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 double free of stun session #2709

Merged
merged 2 commits into from
May 17, 2021
Merged

Fix double free of stun session #2709

merged 2 commits into from
May 17, 2021

Conversation

sauwming
Copy link
Member

@sauwming sauwming commented May 12, 2021

To fix #2505

Upon investigation, the double free issue is caused by a race condition between pj_stun_session_destroy() and on_cache_timeout(). The race is as follows:

  • pj_stun_session_destroy() will call destroy_tdata() and decrement the session's group lock.
  • on_cache_timeout() will also call destroy_tdata() and decrement the session's group lock again.

The patch will make sure that the group lock will not be decremented more than once in such case.

Special thanks to Joshua Colp (@jcolp) for his help in testing the patch.
https://gerrit.asterisk.org/c/asterisk/+/15869

@sauwming
Copy link
Member Author

Regarding the latest commit, it's to fix this issue found during testing:

When called from stun_tsx_on_complete(), destroy_tdata(tdata, PJ_FALSE) will mark tdata->is_destroying to PJ_TRUE, but will actually delay the destruction and call pj_stun_client_tsx_schedule_destroy(tdata->client_tsx, &delay) instead.

So the next call to destroy_tdata() will never do anything as it will return in if (tdata->is_destroying) return. Then since it never erases tdata from the list, this will cause an infinite loop such as here:

    while (!pj_list_empty(&sess->pending_request_list)) {
	pj_stun_tx_data *tdata = sess->pending_request_list.next;
	destroy_tdata(tdata, PJ_TRUE);
    }

@sauwming sauwming merged commit f0ff581 into master May 17, 2021
@sauwming sauwming deleted the stun-race branch May 17, 2021 01:56
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request May 17, 2021
In some cases it was possible for a STUN packet to be destroyed
prematurely or even destroyed partially multiple times.

This patch provided by Teluu fixes the lifetime of these
packets and ensures they aren't partially destroyed multiple
times.

pjsip/pjproject#2709

ASTERISK-29377

Change-Id: Ie842ad24ddf345e01c69a4d333023f05f787abca
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request May 17, 2021
In some cases it was possible for a STUN packet to be destroyed
prematurely or even destroyed partially multiple times.

This patch provided by Teluu fixes the lifetime of these
packets and ensures they aren't partially destroyed multiple
times.

pjsip/pjproject#2709

ASTERISK-29377

Change-Id: Ie842ad24ddf345e01c69a4d333023f05f787abca
asteriskteam pushed a commit to asterisk/asterisk that referenced this pull request May 17, 2021
In some cases it was possible for a STUN packet to be destroyed
prematurely or even destroyed partially multiple times.

This patch provided by Teluu fixes the lifetime of these
packets and ensures they aren't partially destroyed multiple
times.

pjsip/pjproject#2709

ASTERISK-29377

Change-Id: Ie842ad24ddf345e01c69a4d333023f05f787abca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double free in stun session (on_cache_timeout)
3 participants