Skip to content

Commit 1521068

Browse files
bzbarsky-applepull[bot]
authored andcommitted
Make it easier to catch bugs in async command handling. (#25402)
If GetSubjectDescriptor or GetAccessingFabricIndex were called on a CommandHandler after the command handling had gone async, it would sometimes crash, and sometimes not, depending on whether sessions had been evicted or not. Make that situation always crash, so that it will be caught more easily in testing.
1 parent 310be42 commit 1521068

File tree

2 files changed

+25
-1
lines changed

2 files changed

+25
-1
lines changed

src/app/CommandHandler.cpp

+3
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ void CommandHandler::OnInvokeCommandRequest(Messaging::ExchangeContext * ec, con
9393
StatusResponse::Send(status, mExchangeCtx.Get(), false /*aExpectResponse*/);
9494
mSentStatusResponse = true;
9595
}
96+
97+
mGoneAsync = true;
9698
}
9799

98100
Status CommandHandler::ProcessInvokeRequest(System::PacketBufferHandle && payload, bool isTimedInvoke)
@@ -577,6 +579,7 @@ TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter()
577579

578580
FabricIndex CommandHandler::GetAccessingFabricIndex() const
579581
{
582+
VerifyOrDie(!mGoneAsync);
580583
return mExchangeCtx->GetSessionHandle()->GetFabricIndex();
581584
}
582585

src/app/CommandHandler.h

+22-1
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,13 @@ class CommandHandler : public Messaging::ExchangeDelegate
182182
CHIP_ERROR PrepareStatus(const ConcreteCommandPath & aCommandPath);
183183
CHIP_ERROR FinishStatus();
184184
TLV::TLVWriter * GetCommandDataIBTLVWriter();
185+
186+
/**
187+
* GetAccessingFabricIndex() may only be called during synchronous command
188+
* processing. Anything that runs async (while holding a
189+
* CommandHandler::Handle or equivalent) must not call this method, because
190+
* it will not work right if the session we're using was evicted.
191+
*/
185192
FabricIndex GetAccessingFabricIndex() const;
186193

187194
/**
@@ -272,7 +279,17 @@ class CommandHandler : public Messaging::ExchangeDelegate
272279
msgContext->FlushAcks();
273280
}
274281

275-
Access::SubjectDescriptor GetSubjectDescriptor() const { return mExchangeCtx->GetSessionHandle()->GetSubjectDescriptor(); }
282+
/**
283+
* GetSubjectDescriptor() may only be called during synchronous command
284+
* processing. Anything that runs async (while holding a
285+
* CommandHandler::Handle or equivalent) must not call this method, because
286+
* it might not work right if the session we're using was evicted.
287+
*/
288+
Access::SubjectDescriptor GetSubjectDescriptor() const
289+
{
290+
VerifyOrDie(!mGoneAsync);
291+
return mExchangeCtx->GetSessionHandle()->GetSubjectDescriptor();
292+
}
276293

277294
private:
278295
friend class TestCommandInteraction;
@@ -394,6 +411,10 @@ class CommandHandler : public Messaging::ExchangeDelegate
394411
chip::System::PacketBufferTLVWriter mCommandMessageWriter;
395412
TLV::TLVWriter mBackupWriter;
396413
bool mBufferAllocated = false;
414+
// If mGoneAsync is true, we have finished out initial processing of the
415+
// incoming invoke. After this point, our session could go away at any
416+
// time.
417+
bool mGoneAsync = false;
397418
};
398419

399420
} // namespace app

0 commit comments

Comments
 (0)