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

Locking fix so that SSL_shutdown and SSL_write are not called at same time #3583

Merged
merged 2 commits into from
May 30, 2023

Conversation

creslin2877
Copy link
Contributor

@creslin2877 creslin2877 commented May 24, 2023

Adds locking to prevent simultaneous execution of SSL_write and SSL_shutdown. OpenSSL documentation says that multiple threads simultaneously calling SSL_* related functions on the same SSL object is not supported.

It'd be nice if this bug fix got into 2.13.x as well.

Fix for #3575

…hutdown. OpenSSL documentation says that multiple threads simultaneously calling SSL_* related functions on the same SSL object is not supported.
@CLAassistant
Copy link

CLAassistant commented May 24, 2023

CLA assistant check
All committers have signed the CLA.

pjlib/src/pj/ssl_sock_ossl.c Outdated Show resolved Hide resolved
@creslin2877
Copy link
Contributor Author

Any chance of this making it into a 2.13.x release?

@sauwming
Copy link
Member

sauwming commented Jun 2, 2023

Coincidentally, we have been planning to release 2.13.1 to include the recent security fixes: https://github.com/pjsip/pjproject/security/advisories

So yes, we can include this as well (and probably #3577 too?)

@creslin2877
Copy link
Contributor Author

That sounds good. Just to update, so far I haven't seen any crashes in this code on our 30'ish Asterisk instances it's deployed to so far (and we'd have seen a few by now), so I think it's pretty safe. This is also good, as it strongly looks like this bug was probably causing general heap/memory corruption as well, which can manifest itself as lots of weird bugs.

I didn't see any response in #3577 about not calling SSL_shutdown when reading an SSL_ERROR_SSL from an SSL_read() call (in the openssl adapter code for TLS connected SIP). Should I make a separate code review to fix that issue or is @trengginas going to take care of that?

@sauwming
Copy link
Member

sauwming commented Jun 3, 2023

Sorry for the delay, I have merged your fix with #3577.

One more thing, I usually inform George Joseph or Joshua Colp prior to any release, so I take it you will be the one informing them?

@creslin2877
Copy link
Contributor Author

Probably yes - deepest apologies - I actually don't work at Sangoma/Digium anymore, so it might be best to do any pre-release discussions you typically have with them without me.

@sauwming sauwming modified the milestones: release-2.14, release-2.13.1 Jul 5, 2023
dshamaev-intermedia pushed a commit to intermedia-net/pjproject that referenced this pull request Sep 4, 2023
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.

Crash in ssl_write with threading and TLS connections
4 participants