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

Assertion on call hangup from DTMF callback #3970

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

nanangizz
Copy link
Member

To fix #3956.

Stream & transport still access their internal states after being destroyed by the call hangup invoked from within transport callback.

The fix is to move pool release to the transport's group lock handler. Also for added protection, add check for UDP transport validity after invoking callback.

Thanks @R-Jeske for the detailed report.

pjmedia/src/pjmedia/stream.c Show resolved Hide resolved
@nanangizz nanangizz merged commit 2e8f361 into master May 27, 2024
36 checks passed
@nanangizz nanangizz deleted the hangup-from-dtmf-cb branch May 27, 2024 03:19
@R-Jeske
Copy link
Contributor

R-Jeske commented May 27, 2024

With this bugfix, we were not able to reproduce the issue #3956 anymore.

Thanks for the quick response and bugfix.

dshamaev-intermedia added a commit to intermedia-net/pjproject that referenced this pull request Jun 12, 2024
* Add missing openssl SECLEVEL=0 support (pjsip#3890)

Previous SECLEVEL support allowed for levels 1-5.
However, openssl defines levels 0-5. [1]

Recent openssl versions (3.0+) have moved previous
popular ciphers/key lengths (i.e. RSA1024withSHA1)
into level 0, so it is now a reasonable choice to use.

Add support for level 0.

[1] https://www.openssl.org/docs/man3.2/man3/SSL_CTX_set_security_level.html

* Enable Late Offer Answer Mode (LOAM) feature in the pjsua (pjsip#3869)

* Fix warnings for 32-bit compiler and misc fixes. (pjsip#3896)

* Add some missing unlocks (pjsip#3893)

* Prevent race condition in DTLS media stop (pjsip#3901)

* Fix data race reported by ThreadSanitizer in caching pool (pjsip#3897)

* Fixed Metal renderer memory leak (pjsip#3909)

* Fixed DTLS clock stoppage race (pjsip#3905)

* Improve IP address change IPv4 <-> IPv6 (pjsip#3910)

* pjsua_acc: Fix warnings for comparison between ‘pjsua_nat64_opt’ and ‘enum pjsua_ipv6_use’ (pjsip#3915)

* Fix to ext_fmts accessed out of stack scope. (pjsip#3916)

* Add check in siprtp sample app for inactive audio media (pjsip#3927)

* Add function to initialize MediaFormat audio & video (pjsip#3925)

* Fixed incorrect SDP buffer length calculation (pjsip#3924)

* Support Push Notification in iOS sample app (pjsip#3913)

* Fixed PJSUA2 API to get/set Opus config (pjsip#3935)

* Fix bad address length check in pj_ioqueue_sendto(). (pjsip#3941)

* Fix warning of uninitialized value in fuzz-crypto (pjsip#3946)

* Print log on successful send (pjsip#3942)

* Fixed CI Mac build failure (pjsip#3947)

* Update Android JNI audio dev to use 16bit PCM only (pjsip#3945)

* Add TLS/SSL backend: Windows Schannel (pjsip#3867)

* pjsip_find_msg: Log warning if Content-Length field not found (pjsip#3960)

* Fix audiodev index (pjsip#3962)

* Fix assertion on call hangup from DTMF callback (pjsip#3970)

* Fix yaml error in github feature template (pjsip#3972)

* Fix version string in Python setup (pjsip#3976)

* Prevent pjmedia_codec_param.info.enc_ptime_denum division by zero in stream (pjsip#3975)

---------

Co-authored-by: naf <[email protected]>
Co-authored-by: Goodicus <[email protected]>
Co-authored-by: Amilcar Ubiera <[email protected]>
Co-authored-by: Santiago De la Cruz <[email protected]>
Co-authored-by: sauwming <[email protected]>
Co-authored-by: Nanang Izzuddin <[email protected]>
Co-authored-by: dshamaev-intermedia <[email protected]>
Co-authored-by: CI Bot <[email protected]>
Co-authored-by: Pau Espin Pedrol <[email protected]>
Co-authored-by: Riza Sulistyo <[email protected]>
Co-authored-by: Andreas Peldszus <[email protected]>
@sauwming
Copy link
Member

There may be an issue with the stream using the media transport's group lock since their lifetime can be different. For every media update, stream can be destroyed and recreated, but since it's tied to transport, now it won't be destroyed.

To test the issue: make a call, repeatedly hold and reinvite.
Only by the end of the call, will all those streams be destroyed.

11:27:20.032         udp0x12df1ed50  ..UDP media transport destroyed
11:27:20.032        srtp0x12e044e00  ..SRTP transport destroyed
11:27:20.032        strm0x10e817828  ..Stream destroyed
11:27:20.032        strm0x10e2cdc28  ..Stream destroyed
11:27:20.032        strm0x12e91aa28  ..Stream destroyed
11:27:20.032        strm0x12eb4be28  ..Stream destroyed
11:27:20.032        strm0x12e9a4028  ..Stream destroyed

@nanangizz
Copy link
Member Author

If the stream creates its own group lock, and every callback is protected (add/dec ref before/after the callback), will it still solve the original issue #3956?

@sauwming
Copy link
Member

Yeah, I think it should work.

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.

Hangup inside a DTMF callback causes assertion
4 participants