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

Race condition in SSL socket server #2716

Closed
wants to merge 2 commits into from
Closed

Conversation

nanangizz
Copy link
Member

There were a couple reports about crash related to SSL socket. In the investigation, we found a silly bug in SSL socket server code: the configured group lock is not applied to underlying socket (active socket), so race condition may happen between callback and destroy.

Also, call stack in one report points to certificate verification callback (verify_cb() function), which may also be caused by above bug or something else (e.g: OpenSSL application data index acquired via SSL_get_ex_new_index() somehow gets corrupted), so this PR also address the issue by adding normal validation check of SSL socket instance (was only assertion).

…socket server.

- Replace assertion with normal validation check of SSL socket instance in OpenSSL verification callback (verify_cb()) to avoid crash, e.g: if somehow race condition with SSL socket destroy happens or OpenSSL application data index somehow gets corrupted.
@nanangizz nanangizz closed this Jun 15, 2021
@nanangizz nanangizz deleted the ssl-premature-destroy branch June 15, 2021 04:15
@trengginas trengginas restored the ssl-premature-destroy branch July 23, 2021 02:16
@trengginas trengginas reopened this Jul 23, 2021
@trengginas trengginas changed the base branch from master to support-2.11.1 July 23, 2021 02:18
@trengginas trengginas changed the base branch from support-2.11.1 to master July 23, 2021 02:49
@trengginas
Copy link
Member

trengginas commented Jul 23, 2021

Should not reopen this.

@trengginas trengginas closed this Jul 23, 2021
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.

None yet

3 participants