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

Jump to bottom

Fix transport loop #3773

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Fix transport loop #3773

merged 1 commit into from
Nov 24, 2023

Conversation

wosrediinanatour
Copy link
Contributor

@wosrediinanatour wosrediinanatour commented Nov 7, 2023 •
edited

Transport "loop" need set the base.grp_lock, as also shown in transport_adapter_sample.c to have destroying of the loop transport working.

Issue #3771

trengginas
trengginas reviewed Nov 7, 2023
View reviewed changes
pjmedia/src/pjmedia/transport_loop.c Outdated
@@ -239,7 +240,6 @@ static void tp_loop_on_destroy(void *arg)
struct transport_loop *loop = (struct transport_loop*) arg;

PJ_LOG(4, (loop->base.name, "Loop transport destroyed"));
pj_pool_release(loop->pool);
Copy link
Member

Choose a reason for hiding this comment

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

No need to erase this, it will cause leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But then it the pool is released twice: the second time in

pjproject/pjlib/src/pj/lock.c

Line 404 in ca52557

pj_pool_release(pool);

which leads to a crash.

Copy link
Member

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 pool is released when the tp is still in use. If the tp is not used any longer, then it will be safe to release the pool. Tested here without removing the pool release, and there's no crash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I missed that there are two pools...

I have to continue debugging...

Anyway... due to 6dc9b8c I get a crash for a code that worked at least since 2.11.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right - it is working for me too.

@wosrediinanatour
Fix transport loop
15fdec8 
Transport "loop" need set the `base.grp_lock`, as also shown in
`transport_adapter_sample.c` to have destroying of the loop transport
working.

Issue pjsip#3771
@trengginas trengginas added this to the release-2.15 milestone Nov 9, 2023
@nanangizz
Copy link
Member

IIRC the group lock of transport loop is not exported to base.grp_lock on purpose, i.e: it does not really have socket/ioqueue key and it destroys synchronously (not like ICE which may destroy async-ly for TURN deallocation). After the stream/transport is destroyed, I assume the app does not send any packets (and as it is a loop tp, it also won't receive any packet, so there should be no race condition between receive & destroy). So if you see a crash, perhaps the problem is somewhere else. Unfortunately the issue and this PR do not seem to describe the problem detail, e.g: race condition causing use-after-free.

@wosrediinanatour
Copy link
Contributor Author

@nanangizz: In

pjproject/pjmedia/src/pjmedia/transport_loop.c

Line 186 in 9287ac2

status = pj_grp_lock_create(pool, NULL, &grp_lock);
the grp_lock is created, but the pointer grp_lock is nowhere stored in this function.

pjproject/pjmedia/src/pjmedia/transport_loop.c

Line 254 in 9287ac2

pj_grp_lock_dec_ref(tp->grp_lock);
tries to accesstp->grp_lock - which was never set.
E.g. the pool address is stored by tp->pool = pool; at

pjproject/pjmedia/src/pjmedia/transport_loop.c

Line 180 in 9287ac2

tp->pool = pool;
.

nanangizz
nanangizz approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@nanangizz nanangizz left a comment

Choose a reason for hiding this comment

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

@wosrediinanatour You're absolutely right! Thanks for pointing it out.

@nanangizz nanangizz merged commit f9ed97b into pjsip:master Nov 24, 2023
29 of 35 checks passed
@sauwming sauwming linked an issue Nov 28, 2023 that may be closed by this pull request
Loop transport does not work with PJSUA(2) #3771
Closed
@sauwming sauwming mentioned this pull request Nov 28, 2023
Closed
trengginas pushed a commit that referenced this pull request Mar 11, 2024
@wosrediinanatour @trengginas
Fix transport loop (#3773)
7cda346 
Transport "loop" need set the `base.grp_lock`, as also shown in
`transport_adapter_sample.c` to have destroying of the loop transport
working.

Issue #3771
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trengginas trengginas trengginas approved these changes

@sauwming sauwming sauwming approved these changes

@nanangizz nanangizz nanangizz approved these changes

Assignees
No one assigned
Labels
component: pjmedia type: bug
Projects
None yet
Milestone
release-2.14.1
Development

Successfully merging this pull request may close these issues.

Loop transport does not work with PJSUA(2)
4 participants
@wosrediinanatour @nanangizz @trengginas @sauwming