Skip to content

Commit

Permalink
Fixed BindingManager to make sure it doesn't use invalid context
Browse files Browse the repository at this point in the history
BindingManager uses application context passed to
NotifyBoundClusterChanged and doesn't care about its life time.
Applications delete the context after calling the method, so if
the context will be used asynchronously (like for HandleDeviceConnect)
it may have invalid value.

* Added for BindingManager method that allows to register handler
called when context is not used anymore and can be released.
* Added releasing context in applications using context release
handler.
  • Loading branch information
kkasperczyk-no committed Apr 14, 2022
1 parent 01e54b2 commit bd062f8
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,18 @@ static void BoundDeviceChangedHandler(const EmberBindingTableEntry & binding, ch
}
}

static void BoundDeviceContextReleaseHandler(void * context)
{
(void) context;
}

static void InitBindingHandlerInternal(intptr_t arg)
{
auto & server = chip::Server::GetInstance();
chip::BindingManager::GetInstance().Init(
{ &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() });
chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler);
chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(BoundDeviceContextReleaseHandler);
}

CHIP_ERROR InitBindingHandlers()
Expand Down
10 changes: 8 additions & 2 deletions examples/light-switch-app/efr32/src/binding-handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,13 @@ void LightSwitchChangedHandler(const EmberBindingTableEntry & binding, DevicePro
}
}

void LightSwitchContextReleaseHandler(void * context)
{
VerifyOrReturn(context != nullptr, ChipLogError(NotSpecified, "OnDeviceConnectedFn: context is null"));
BindingCommandData * data = static_cast<BindingCommandData *>(context);
Platform::Delete(data);
}

#ifdef ENABLE_CHIP_SHELL

/********************************************************
Expand Down Expand Up @@ -384,6 +391,7 @@ void InitBindingHandlerInternal(intptr_t arg)
chip::BindingManager::GetInstance().Init(
{ &server.GetFabricTable(), server.GetCASESessionManager(), &server.GetPersistentStorage() });
chip::BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler);
chip::BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler);
}

} // namespace
Expand All @@ -398,8 +406,6 @@ void SwitchWorkerFunction(intptr_t context)

BindingCommandData * data = reinterpret_cast<BindingCommandData *>(context);
BindingManager::GetInstance().NotifyBoundClusterChanged(data->localEndpointId, data->clusterId, static_cast<void *>(data));

Platform::Delete(data);
}

void BindingWorkerFunction(intptr_t context)
Expand Down
16 changes: 13 additions & 3 deletions examples/light-switch-app/nrfconnect/main/BindingHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,13 @@ void BindingHandler::OnInvokeCommandFailure(DeviceProxy * aDevice, BindingData &
// Set flag to not try recover session multiple times.
BindingHandler::GetInstance().mCaseSessionRecovered = true;

// Allocate new object to make sure its life time will be appropriate.
BindingHandler::BindingData * data = Platform::New<BindingHandler::BindingData>();
*data = aBindingData;

// Establish new CASE session and retrasmit command that was not applied.
error = BindingManager::GetInstance().NotifyBoundClusterChanged(aBindingData.EndpointId, aBindingData.ClusterId,
static_cast<void *>(&aBindingData));
static_cast<void *>(data));
}
else
{
Expand Down Expand Up @@ -222,6 +226,13 @@ void BindingHandler::LightSwitchChangedHandler(const EmberBindingTableEntry & bi
}
}

void BindingHandler::LightSwitchContextReleaseHandler(void * context)
{
VerifyOrReturn(context != nullptr, LOG_ERR("Invalid context for Light switch context release handler"););
BindingData * data = static_cast<BindingData *>(context);
Platform::Delete(data);
}

void BindingHandler::InitInternal(intptr_t aArg)
{
LOG_INF("Initialize binding Handler");
Expand All @@ -234,6 +245,7 @@ void BindingHandler::InitInternal(intptr_t aArg)
}

BindingManager::GetInstance().RegisterBoundDeviceChangedHandler(LightSwitchChangedHandler);
BindingManager::GetInstance().RegisterBoundDeviceContextReleaseHandler(LightSwitchContextReleaseHandler);
BindingHandler::GetInstance().PrintBindingTable();
}

Expand Down Expand Up @@ -298,6 +310,4 @@ void BindingHandler::SwitchWorkerHandler(intptr_t aContext)
BindingData * data = reinterpret_cast<BindingData *>(aContext);
LOG_INF("Notify Bounded Cluster | endpoint: %d cluster: %d", data->EndpointId, data->ClusterId);
BindingManager::GetInstance().NotifyBoundClusterChanged(data->EndpointId, data->ClusterId, static_cast<void *>(data));

Platform::Delete(data);
}
2 changes: 0 additions & 2 deletions examples/light-switch-app/nrfconnect/main/LightSwitch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ void LightSwitch::InitiateActionSwitch(Action mAction)
}
data->IsGroup = BindingHandler::GetInstance().IsGroupBound();
DeviceLayer::PlatformMgr().ScheduleWork(BindingHandler::SwitchWorkerHandler, reinterpret_cast<intptr_t>(data));
Platform::Delete(data);
}
}

Expand All @@ -79,6 +78,5 @@ void LightSwitch::DimmerChangeBrightness()
data->Value = (uint8_t) sBrightness;
data->IsGroup = BindingHandler::GetInstance().IsGroupBound();
DeviceLayer::PlatformMgr().ScheduleWork(BindingHandler::SwitchWorkerHandler, reinterpret_cast<intptr_t>(data));
Platform::Delete(data);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class BindingHandler
static void OnOffProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *);
static void LevelControlProcessCommand(chip::CommandId, const EmberBindingTableEntry &, chip::DeviceProxy *, void *);
static void LightSwitchChangedHandler(const EmberBindingTableEntry &, chip::DeviceProxy *, void *);
static void LightSwitchContextReleaseHandler(void * context);
static void InitInternal(intptr_t);

bool mCaseSessionRecovered = false;
Expand Down
16 changes: 16 additions & 0 deletions src/app/clusters/bindings/BindingManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ void BindingManager::HandleDeviceConnected(OperationalDeviceProxy * device)
fabricToRemove = entry.fabricIndex;
nodeToRemove = entry.nodeId;
mBoundDeviceChangedHandler(entry, device, pendingNotification.mContext);
mBoundDeviceContextReleaseHandler(pendingNotification.mContext);
}
}
mPendingNotificationMap.RemoveAllEntriesForNode(fabricToRemove, nodeToRemove);
Expand All @@ -176,6 +177,19 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err
{
// Simply release the entry, the connection will be re-established as needed.
ChipLogError(AppServer, "Failed to establish connection to node 0x" ChipLogFormatX64, ChipLogValueX64(peerId.GetNodeId()));

for (PendingNotificationEntry pendingNotification : mPendingNotificationMap)
{
EmberBindingTableEntry entry = BindingTable::GetInstance().GetAt(pendingNotification.mBindingEntryId);

PeerId peer = PeerIdForNode(mInitParams.mFabricTable, entry.fabricIndex, entry.nodeId);

if (peerId == peer)
{
mBoundDeviceContextReleaseHandler(pendingNotification.mContext);
}
}

mInitParams.mCASESessionManager->ReleaseSession(peerId);
}

Expand Down Expand Up @@ -204,6 +218,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste
{
// We already have an active connection
mBoundDeviceChangedHandler(*iter, peerDevice, context);
mBoundDeviceContextReleaseHandler(context);
}
else
{
Expand All @@ -214,6 +229,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste
else if (iter->type == EMBER_MULTICAST_BINDING)
{
mBoundDeviceChangedHandler(*iter, nullptr, context);
mBoundDeviceContextReleaseHandler(context);
}
}
}
Expand Down
13 changes: 13 additions & 0 deletions src/app/clusters/bindings/BindingManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ namespace chip {
*/
using BoundDeviceChangedHandler = void (*)(const EmberBindingTableEntry & binding, DeviceProxy * peer_device, void * context);

using BoundDeviceContextReleaseHandler = void (*)(void * context);

struct BindingManagerInitParams
{
FabricTable * mFabricTable = nullptr;
Expand Down Expand Up @@ -70,6 +72,16 @@ class BindingManager

void RegisterBoundDeviceChangedHandler(BoundDeviceChangedHandler handler) { mBoundDeviceChangedHandler = handler; }

/*
* Registers handler that will be called when context used in NotifyBoundClusterChanged will not be needed and could be
* released.
*
*/
void RegisterBoundDeviceContextReleaseHandler(BoundDeviceContextReleaseHandler handler)
{
mBoundDeviceContextReleaseHandler = handler;
}

CHIP_ERROR Init(const BindingManagerInitParams & params);

/*
Expand Down Expand Up @@ -117,6 +129,7 @@ class BindingManager

PendingNotificationMap mPendingNotificationMap;
BoundDeviceChangedHandler mBoundDeviceChangedHandler;
BoundDeviceContextReleaseHandler mBoundDeviceContextReleaseHandler;
BindingManagerInitParams mInitParams;

Callback::Callback<OnDeviceConnected> mOnConnectedCallback;
Expand Down

0 comments on commit bd062f8

Please sign in to comment.