From e0f1cd7621901c3348fd2497c8f7ef6dd2996465 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 24 Feb 2026 14:58:53 -0500 Subject: [PATCH 1/2] type-context.h: Extent cancel_mutex lock to prevent theoretical race As pointed out by janb84 in https://github.com/bitcoin/bitcoin/pull/34422#discussion_r2849037119 it makes sense for the on_cancel callback to lock cancel_mutex while it is assigning request_canceled = true. The lock and assigment were introduced in #240 and in an earlier version of that PR, request_canceled was a std::atomic and the assignment happened before the lock was acquired instead of after, so it was ok for the lock to be unnamed and immediately released after being acquired. But in the final verion of #240 request_canceled is an ordinary non-atomic bool, and it should be assigned true with the lock held to prevent a theoretical race condition where capn'proto event loop cancels the request before the execution thread runs, and the execution thread sees the old request_canceled = false value and then unsafely accesses deleted parameters. The request being canceled so quickly and parameters being accessed so slowly, and stale request_canceled value being read even after the execution thread has the cancel_mutex lock should be very unlikely to occur in practice, but could happen in theory and is good to fix. --- include/mp/type-context.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/mp/type-context.h b/include/mp/type-context.h index 3733d6b..109aff1 100644 --- a/include/mp/type-context.h +++ b/include/mp/type-context.h @@ -123,7 +123,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& // it. So in addition to locking the mutex, the // execution thread always checks request_canceled // as well before accessing the structs. - Lock{cancel_mutex}; + Lock cancel_lock{cancel_mutex}; server_context.request_canceled = true; }; // Update requests_threads map if not canceled. From ef96a5b2be2f35ef09c19a5ff4a113144c596309 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 20 Feb 2026 17:20:02 -0500 Subject: [PATCH 2/2] doc: Comment cleanups after #240 --- include/mp/type-context.h | 2 +- include/mp/util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/mp/type-context.h b/include/mp/type-context.h index 109aff1..72c3963 100644 --- a/include/mp/type-context.h +++ b/include/mp/type-context.h @@ -210,7 +210,7 @@ auto PassField(Priority<1>, TypeList<>, ServerContext& server_context, const Fn& // connection is destroyed. (By default Cap'n Proto does not cancel requests // on disconnect, since it's possible clients might want to make requests // and immediately disconnect without waiting for results, but not want the - // the requests to be canceled.) + // requests to be canceled.) return server.m_context.connection->m_canceler.wrap(kj::mv(result)); } } // namespace mp diff --git a/include/mp/util.h b/include/mp/util.h index 80791bd..a3db128 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -331,7 +331,7 @@ void CancelMonitor::promiseDestroyed(CancelProbe& probe) // theory this method could be called when a promise was fulfilled or // rejected rather than canceled, but it's safe to assume that's not the // case because the CancelMonitor class is meant to be used inside code - // fulfilling or rejecting the promise and destroyed before doing these. + // fulfilling or rejecting the promise and destroyed before doing so. assert(m_probe == &probe); m_canceled = true; if (m_on_cancel) m_on_cancel();