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

Add some missing unlocks #3893

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Add some missing unlocks #3893

merged 2 commits into from
Mar 26, 2024

Conversation

xhit
Copy link
Contributor

@xhit xhit commented Mar 21, 2024

Add some missing unlocks in the code.

Untested in local, I found some playing with the library, If some are unnecessary we can to edit.

nanangizz
nanangizz reviewed Mar 22, 2024
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.

Good catch, thanks.
Just a couple notes.

pjlib-util/src/pjlib-util/resolver.c Outdated
if (pkt->ans[0].name.slen >= PJ_MAX_HOSTNAME){
pj_grp_lock_release(resolver->grp_lock);
return PJ_ENAMETOOLONG;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should maintain it as an assertion check, for example:

PJ_ASSERT_ON_FAIL(pkt->ans[0].name.slen < PJ_MAX_HOSTNAME, 
                  { pj_grp_lock_release(resolver->grp_lock); return PJ_ENAMETOOLONG; });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ee6beea

pjlib-util/src/pjlib-util/resolver.c Outdated
pj_grp_lock_release(resolver->grp_lock);
return PJ_ENAMETOOLONG;
}

Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea I head when seeing this PR: In other places a return is "deferred" in case of error:

//...

if (error) 
   goto on_error;

//...
return;

on_error:
   //... some cleanup ...

return;

This is usually quite useful with locks in C since there is nothing like RAII.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ee6beea

@nanangizz nanangizz added this to the release-2.15 milestone Mar 22, 2024
@sauwming sauwming merged commit 427a2b3 into pjsip:master Mar 26, 2024
36 checks passed
dshamaev-intermedia pushed a commit to intermedia-net/pjproject that referenced this pull request Apr 4, 2024
@xhit @dshamaev-intermedia
Add some missing unlocks (pjsip#3893)
e37d4e2
dshamaev-intermedia pushed a commit to intermedia-net/pjproject that referenced this pull request Apr 4, 2024
@xhit @dshamaev-intermedia
Add some missing unlocks (pjsip#3893)
ca3dba6
dshamaev-intermedia added a commit to intermedia-net/pjproject that referenced this pull request Apr 4, 2024
@github-actions @naf419
@goodicus @amubiera @xhit @sauwming @nanangizz @dshamaev-intermedia
Automated Pull Request (#77)
d2d79d1 
* 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)

---------

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]>
dshamaev-intermedia added a commit to intermedia-net/pjproject that referenced this pull request Apr 5, 2024
@dshamaev-intermedia @nanangizz
@sauwming @xhit @andreas-wehrmann @trengginas
Feature/apps 17701 ipv6 and ip change fixes from 2.15 (#78)
83a228f 
* Add option to shutdown all transports on IP change (pjsip#3781)

* pjsua_handle_ip_change: Added missing null check for on_ip_changed_progress callback (pjsip#3830)

* Reset stored remote name in dialog (dlg->initial_dest) if transport is server. (pjsip#3783)

* Prevent immediate tsx termination upon transport error (pjsip#3805)

* Fixed issues when adding new media and deinitializing media (pjsip#3821)

* Potential issues when IPv6 is disabled (pjsip#3835)

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

* Add some missing unlocks (pjsip#3893)

* add missing unlock (pjsip#3885)

* Retransmit 2xx response when transport is closed (pjsip#3828)

* Fixed account's route set update when modifying account (pjsip#3825)

---------

Co-authored-by: Nanang Izzuddin <[email protected]>
Co-authored-by: sauwming <[email protected]>
Co-authored-by: Santiago De la Cruz <[email protected]>
Co-authored-by: Andreas Wehrmann <[email protected]>
Co-authored-by: Riza Sulistyo <[email protected]>
dshamaev-intermedia added a commit to intermedia-net/pjproject that referenced this pull request Jun 12, 2024
@github-actions @naf419
@goodicus @amubiera @xhit @sauwming @nanangizz @dshamaev-intermedia @pespin @trengginas @peldszus
Automated Pull Request (#86)
e9b9e72 
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wosrediinanatour wosrediinanatour wosrediinanatour left review comments

@nanangizz nanangizz nanangizz approved these changes

@sauwming sauwming sauwming approved these changes

@trengginas trengginas Awaiting requested review from trengginas

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

Successfully merging this pull request may close these issues.
4 participants
@xhit @sauwming @nanangizz @wosrediinanatour