Close small race condition on error raising in QUIC #28642
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Github issue openssl#28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again.
nhorman
requested review from
mattcaswell and
t8m
and removed request for
mattcaswell
t8m
added
branch: master
t8m
approved these changes
Sep 23, 2025
t8m
added
approval: done
npajkovsky
approved these changes
Sep 23, 2025
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
openssl-machine
pushed a commit
that referenced
this pull request
Sep 24, 2025
Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again. Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28642) (cherry picked from commit 1e70e80)
openssl-machine
pushed a commit
that referenced
this pull request
Sep 24, 2025
Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again. Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28642) (cherry picked from commit 1e70e80)
openssl-machine
pushed a commit
that referenced
this pull request
Sep 24, 2025
Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again. Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28642)
openssl-machine
pushed a commit
that referenced
this pull request
Sep 24, 2025
Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again. Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28642) (cherry picked from commit 1e70e80)
openssl-machine
pushed a commit
that referenced
this pull request
Sep 24, 2025
Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again. Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28642) (cherry picked from commit 1e70e80)
|
Merged to master, 3.6, 3.5, 3.4, 3.3 and 3.2 branches. Thank you. |
openssl-machine
pushed a commit
that referenced
this pull request
Sep 24, 2025
Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack. It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel. Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur. Except for one case, which this PR addresses. In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel. According to the reporter, applying this patch prevents the reported error from occuring again. Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #28642) (cherry picked from commit 1e70e80)
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.5.4 CHANGES.md includes the following: * openssl#28098 * openssl#28415 * openssl#28504 * openssl#28535 * openssl#28569 * openssl#28573 * openssl#28576 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 * openssl#28676 3.5.4 NEWS.md includes the following: * openssl#28603 Updated the changes and news in the previous branches. Removed the attribution in NEWS.md incorrectly introduced in e551da6 "Update news and changes for the 3.5.3 release". Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.4.3 CHANGES.md includes the following: * openssl#28098 * openssl#28415 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.4.3 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.5.4 CHANGES.md includes the following: * openssl#28098 * openssl#28415 * openssl#28504 * openssl#28535 * openssl#28569 * openssl#28573 * openssl#28576 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 * openssl#28676 3.5.4 NEWS.md includes the following: * openssl#28603 Updated the changes and news in the previous branches. Removed the attribution in NEWS.md incorrectly introduced in e551da6 "Update news and changes for the 3.5.3 release". Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.3.5 CHANGES.md includes the following: * openssl#28098 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.3.5 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.2.6 CHANGES.md includes the following: * openssl#28098 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.2.6 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.2.6 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.2.6 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.3.5 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.3.5 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.4.3 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28415 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.4.3 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.5.4 CHANGES.md includes the following: * openssl#28098 * openssl#28415 * openssl#28504 * openssl#28535 * openssl#28569 * openssl#28573 * openssl#28576 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 * openssl#28676 3.5.4 NEWS.md includes the following: * openssl#28603 Updated the changes and news in the previous branches. Removed the attribution in NEWS.md incorrectly introduced in e551da6 "Update news and changes for the 3.5.3 release". Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.5.4 CHANGES.md includes the following: * openssl#28098 * openssl#28415 * openssl#28504 * openssl#28535 * openssl#28569 * openssl#28573 * openssl#28576 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 * openssl#28676 3.5.4 NEWS.md includes the following: * openssl#28603 Updated the changes and news in the previous branches. Removed the attribution in NEWS.md incorrectly introduced in e551da6 "Update news and changes for the 3.5.3 release". Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.3.5 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.3.5 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.4.3 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28415 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.4.3 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
esyr
added a commit
to esyr/openssl
that referenced
this pull request
Sep 30, 2025
3.2.6 CHANGES.md includes the following: * openssl#28098 * openssl#28198 * openssl#28398 * openssl#28411 * openssl#28449 * openssl#28504 * openssl#28535 * openssl#28591 * openssl#28603 * openssl#28624 * openssl#28642 3.2.6 NEWS.md do not have any updates. Updated the changes and news in the previous branches. Release: Yes Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Github issue #28501 reported an odd condition in which a double free was occuring when a given thread was popping entries of its error stack.
It was hypothesized that, because a few places in the quic stack save error state to a shared structure (ch->err_state, port->error_state, qtls->error_state), that multiple threads may attempt to mutate the shared structure during error save/restore in parallel.
Investigation showed that all paths which led to such mutations were done under lock, so that shouldn't occur.
Except for one case, which this PR addresses.
In ossl_quic_conn_stream_conclude, we unlock our protecting mutex, prior to calling QUIC_RAISE_NON_NORMAL_ERROR. If that function is called with an reason code of SHUTDOWN, it attempts to restore the channel error state. Given that the lock was released first, this creates a small race condition in which two threads may manipulate the shared error state in the channel struct in parallel.
According to the reporter, applying this patch prevents the reported error from occuring again.