From 6b8aef94a540a56773f039cbb5cfee86229a3638 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 15 Dec 2021 14:53:01 -0500 Subject: [PATCH] Fix double-free bugs on failure to send a write message. There were two separate bugs here: 1) For a group write, we were violating the contract for WriteClient::SendWriteRequest, which is that the caller must call Shutdown on failure but the WriteClient itself will do it on success (and call OnDone in the process). We were unconditionally calling Shutdown and OnDone inside SetWriteRequest, even on failure. 2) WriteInteraction was violating the contract for WriteClientHandle::SendWriteRequest, which is different: it always guarantees it will call OnDone. But the consumer was assuming that OnDone would only be called if SendWriteRequest returned success. --- src/app/WriteClient.cpp | 18 ++++++++++++------ src/app/WriteClient.h | 3 +++ src/controller/WriteInteraction.h | 6 +++++- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index 36501ee780d35f..ce1df1a0b2e633 100644 --- a/src/app/WriteClient.cpp +++ b/src/app/WriteClient.cpp @@ -248,14 +248,20 @@ CHIP_ERROR WriteClient::SendWriteRequest(SessionHandle session, System::Clock::T ChipLogError(DataManagement, "Write client failed to SendWriteRequest"); ClearExistingExchangeContext(); } - - if (session.IsGroupSession()) + else { - // Always shutdown on Group communication - ChipLogDetail(DataManagement, "Closing on group Communication "); + // TODO: Ideally this would happen async, but to make sure that we + // handle this object dying (e.g. due to IM enging shutdown) while the + // async bits are pending we'd need to malloc some state bit that we can + // twiddle if we die. For now just do the OnDone callback sync. + if (session.IsGroupSession()) + { + // Always shutdown on Group communication + ChipLogDetail(DataManagement, "Closing on group Communication "); - // onDone is called - ShutdownInternal(); + // onDone is called + ShutdownInternal(); + } } return err; diff --git a/src/app/WriteClient.h b/src/app/WriteClient.h index 1a9a93dae7ffd3..5151c2732aaa2b 100644 --- a/src/app/WriteClient.h +++ b/src/app/WriteClient.h @@ -248,6 +248,9 @@ class WriteClientHandle /** * Finalize the message and send it to the desired node. The underlying write object will always be released, and the user * should not use this object after calling this function. + * + * Note: In failure cases this will _synchronously_ invoke OnDone on the + * WriteClient::Callback before returning. */ CHIP_ERROR SendWriteRequest(SessionHandle session, System::Clock::Timeout timeout = kImMessageTimeout); diff --git a/src/controller/WriteInteraction.h b/src/controller/WriteInteraction.h index 59c49761484135..037158e8b2cd53 100644 --- a/src/controller/WriteInteraction.h +++ b/src/controller/WriteInteraction.h @@ -106,6 +106,11 @@ CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpoint VerifyOrReturnError(callback != nullptr, CHIP_ERROR_NO_MEMORY); ReturnErrorOnFailure(app::InteractionModelEngine::GetInstance()->NewWriteClient(handle, callback.get(), aTimedWriteTimeoutMs)); + + // At this point the handle will ensure our callback's OnDone is always + // called. + callback.release(); + if (sessionHandle.IsGroupSession()) { ReturnErrorOnFailure( @@ -119,7 +124,6 @@ CHIP_ERROR WriteAttribute(SessionHandle sessionHandle, chip::EndpointId endpoint ReturnErrorOnFailure(handle.SendWriteRequest(sessionHandle)); - callback.release(); return CHIP_NO_ERROR; }