Draft
Conversation
Add test for race condition in makeThread that can currently trigger segfaults as reported: bitcoin/bitcoin#34711 bitcoin/bitcoin#34756 The test currently crashes and will be fixed in the next commit. Co-authored-by: Ryan Ofsky <ryan@ofsky.org> git-bisect-skip: yes
This fixes a race condition in makeThread that can currently trigger segfaults as reported: bitcoin/bitcoin#34711 bitcoin/bitcoin#34756 The bug can be reproduced by running the unit test added in the previous commit or by calling makeThread and immediately disconnecting or destroying the returned thread. The bug is not new and has existed since makeThread was implemented, but it was found due to a new functional test in bitcoin core and with antithesis testing (see details in linked issues). The fix was originally posted in bitcoin/bitcoin#34711 (comment)
Add test for disconnect race condition in the mp.Context PassField() overload that can currently trigger segfaults as reported in bitcoin/bitcoin#34777. The test currently crashes and will be fixed in the next commit. Co-authored-by: Ryan Ofsky <ryan@ofsky.org> git-bisect-skip: yes
This fixes a race condition in the mp.Context PassField() overload which is used to execute async requests, that can currently trigger segfaults as reported in bitcoin/bitcoin#34777 when it calls call_context.getParams() after a disconnect. The bug can be reproduced by running the unit test added in the previous commit and was also seen in antithesis (see details in linked issue), but should be unlikely to happen normally because PassField checks for cancellation and returns early before actually using the getParams() result. This bug was introduced commit in 0174450 which started to cancel requests on disconnects. Before that commit, requests would continue to execute after a disconnect and it was ok to call getParams(). This fix was originally posted in bitcoin/bitcoin#34777 (comment)
Add test disconnect for race condition in the mp.Context PassField() overload reported in bitcoin/bitcoin#34782. The test crashes currently with AddressSanitizer, but will be fixed in the next commit. It's also possible to reproduce the bug without AddressSanitizer by adding an assert: ```diff --- a/include/mp/type-context.h +++ b/include/mp/type-context.h @@ -101,2 +101,3 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& server_context.cancel_lock = &cancel_lock; + KJ_DEFER(server_context.cancel_lock = nullptr); server.m_context.loop->sync([&] { @@ -111,2 +112,3 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& MP_LOG(*server.m_context.loop, Log::Info) << "IPC server request #" << req << " canceled while executing."; + assert(server_context.cancel_lock); // Lock cancel_mutex here to block the event loop ``` Co-authored-by: Ryan Ofsky <ryan@ofsky.org> git-bisect-skip: yes
This fixes a race condition in the mp.Context PassField() overload which is used to execute async requests, that can currently trigger segfaults as reported in bitcoin/bitcoin#34782 when a cancellation happens after the request executes but before it returns. The bug can be reproduced by running the unit test added in the previous commit and was also seen in antithesis (see details in linked issue), but should be unlikely to happen normally because the cancellation would have to happen in a very short window for there to be a problem. This bug was introduced commit in 0174450 which started to cancel requests on disconnects. Before that commit a cancellation callback was not present. This fix was originally posted in bitcoin/bitcoin#34782 (comment)
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. LLM Linter (✨ experimental)Possible typos and grammar issues:
2026-03-13 09:35:59 |
2a3ae34 to
3dd6d57
Compare
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
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.
Testing the race condition fixes in #249 against the Bitcoin Core CI jobs added in #253.