Skip to content

Commit

Permalink
Fix race when trying to use Darwin callback bridge from action block. (…
Browse files Browse the repository at this point in the history 
…project-chip#23553)

We have some callback bridge action blocks that need access to the bridge
itself.  Since the action block was queued to run on a different thread from
inside the bridge constructor, it could race with the bridge constructor
finishing execution, leading to unexpected behavior (including possibly
destruction of the bridge before its constructor finishes executing.

Switch to running the action block after the bridge is constructed.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple authored Nov 9, 2022
1 parent fb865cc commit 1ff789e
Show file tree
Hide file tree
Showing 8 changed files with 32,554 additions and 42,206 deletions.
71 changes: 29 additions & 42 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,6 @@ static CHIP_ERROR MTREncodeTLVFromDataValueDictionary(id object, chip::TLV::TLVW

// Callback type to pass data value as an NSObject
typedef void (*MTRDataValueDictionaryCallback)(void * context, id value);
typedef void (*MTRErrorCallback)(void * context, CHIP_ERROR error);

// Rename to be generic for decode and encode
class MTRDataValueDictionaryDecodableType {
Expand Down Expand Up @@ -689,9 +688,9 @@ CHIP_ERROR Encode(chip::TLV::TLVWriter & writer, chip::TLV::Tag tag) const
// Callback bridge for MTRDataValueDictionaryCallback
class MTRDataValueDictionaryCallbackBridge : public MTRCallbackBridge<MTRDataValueDictionaryCallback> {
public:
MTRDataValueDictionaryCallbackBridge(dispatch_queue_t queue, MTRBaseDevice * device, MTRDeviceResponseHandler handler,
MTRActionBlock action, bool keepAlive = false)
: MTRCallbackBridge<MTRDataValueDictionaryCallback>(queue, device, handler, action, OnSuccessFn, keepAlive) {};
MTRDataValueDictionaryCallbackBridge(
dispatch_queue_t queue, MTRDeviceResponseHandler handler, MTRActionBlock action, bool keepAlive = false)
: MTRCallbackBridge<MTRDataValueDictionaryCallback>(queue, handler, action, OnSuccessFn, keepAlive) {};

static void OnSuccessFn(void * context, id value) { DispatchSuccess(context, value); }
};
Expand Down Expand Up @@ -791,14 +790,9 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
clusterID = (clusterID == nil) ? nil : [clusterID copy];
attributeID = (attributeID == nil) ? nil : [attributeID copy];
params = (params == nil) ? nil : [params copy];
new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
^(ExchangeManager & exchangeManager, const SessionHandle & session, chip::Callback::Cancelable * success,
chip::Callback::Cancelable * failure) {
auto successFn = chip::Callback::Callback<MTRDataValueDictionaryCallback>::FromCancelable(success);
auto failureFn = chip::Callback::Callback<MTRErrorCallback>::FromCancelable(failure);
auto context = successFn->mContext;
auto successCb = successFn->mCall;
auto failureCb = failureFn->mCall;
auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
auto resultArray = [[NSMutableArray alloc] init];
auto resultSuccess = [[NSMutableArray alloc] init];
auto resultFailure = [[NSMutableArray alloc] init];
Expand Down Expand Up @@ -842,23 +836,23 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
readParams.mAttributePathParamsListSize = 1;
readParams.mIsFabricFiltered = params.filterByFabric;

auto onDone = [resultArray, resultSuccess, resultFailure, context, successCb, failureCb](
auto onDone = [resultArray, resultSuccess, resultFailure, bridge, successCb, failureCb](
BufferedReadAttributeCallback<MTRDataValueDictionaryDecodableType> * callback) {
if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
// Failure
if (failureCb) {
if ([resultFailure count] > 0) {
failureCb(context, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
} else if ([resultArray count] > 0) {
failureCb(context, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
} else {
failureCb(context, CHIP_ERROR_READ_FAILED);
failureCb(bridge, CHIP_ERROR_READ_FAILED);
}
}
} else {
// Success
if (successCb) {
successCb(context, resultArray);
successCb(bridge, resultArray);
}
}
chip::Platform::Delete(callback);
Expand Down Expand Up @@ -887,6 +881,7 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
callback.release();
return err;
});
std::move(*bridge).DispatchAction(self);
}

- (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
Expand All @@ -897,14 +892,9 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
queue:(dispatch_queue_t)queue
completion:(MTRDeviceResponseHandler)completion
{
new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
^(ExchangeManager & exchangeManager, const SessionHandle & session, chip::Callback::Cancelable * success,
chip::Callback::Cancelable * failure) {
auto successFn = chip::Callback::Callback<MTRDataValueDictionaryCallback>::FromCancelable(success);
auto failureFn = chip::Callback::Callback<MTRErrorCallback>::FromCancelable(failure);
auto context = successFn->mContext;
auto successCb = successFn->mCall;
auto failureCb = failureFn->mCall;
auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
auto resultArray = [[NSMutableArray alloc] init];
auto resultSuccess = [[NSMutableArray alloc] init];
auto resultFailure = [[NSMutableArray alloc] init];
Expand All @@ -929,22 +919,22 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
};

auto onDoneCb
= [context, successCb, failureCb, resultArray, resultSuccess, resultFailure](app::WriteClient * pWriteClient) {
= [bridge, successCb, failureCb, resultArray, resultSuccess, resultFailure](app::WriteClient * pWriteClient) {
if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
// Failure
if (failureCb) {
if ([resultFailure count] > 0) {
failureCb(context, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
} else if ([resultArray count] > 0) {
failureCb(context, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
} else {
failureCb(context, CHIP_ERROR_WRITE_FAILED);
failureCb(bridge, CHIP_ERROR_WRITE_FAILED);
}
}
} else {
// Success
if (successCb) {
successCb(context, resultArray);
successCb(bridge, resultArray);
}
}
};
Expand All @@ -956,6 +946,7 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
onSuccessCb, onFailureCb, (timeoutMs == nil) ? NullOptional : Optional<uint16_t>([timeoutMs unsignedShortValue]),
onDoneCb, NullOptional);
});
std::move(*bridge).DispatchAction(self);
}

class NSObjectCommandCallback final : public app::CommandSender::Callback {
Expand Down Expand Up @@ -1045,14 +1036,9 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
commandFields = (commandFields == nil) ? nil : [commandFields copy];
timeoutMs = (timeoutMs == nil) ? nil : [timeoutMs copy];

new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
^(ExchangeManager & exchangeManager, const SessionHandle & session, chip::Callback::Cancelable * success,
chip::Callback::Cancelable * failure) {
auto successFn = chip::Callback::Callback<MTRDataValueDictionaryCallback>::FromCancelable(success);
auto failureFn = chip::Callback::Callback<MTRErrorCallback>::FromCancelable(failure);
auto context = successFn->mContext;
auto successCb = successFn->mCall;
auto failureCb = failureFn->mCall;
auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
auto resultArray = [[NSMutableArray alloc] init];
auto resultSuccess = [[NSMutableArray alloc] init];
auto resultFailure = [[NSMutableArray alloc] init];
Expand Down Expand Up @@ -1086,21 +1072,21 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
VerifyOrReturnError(decoder != nullptr, CHIP_ERROR_NO_MEMORY);

auto rawDecoderPtr = decoder.get();
auto onDoneCb = [rawDecoderPtr, context, successCb, failureCb, resultArray, resultSuccess, resultFailure](
auto onDoneCb = [rawDecoderPtr, bridge, successCb, failureCb, resultArray, resultSuccess, resultFailure](
app::CommandSender * commandSender) {
if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
// Failure
if (failureCb) {
if ([resultFailure count] > 0) {
failureCb(context, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
} else {
failureCb(context, CHIP_ERROR_WRITE_FAILED);
failureCb(bridge, CHIP_ERROR_WRITE_FAILED);
}
}
} else {
// Success
if (successCb) {
successCb(context, resultArray);
successCb(bridge, resultArray);
}
}
chip::Platform::Delete(commandSender);
Expand All @@ -1121,6 +1107,7 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
commandSender.release();
return CHIP_NO_ERROR;
});
std::move(*bridge).DispatchAction(self);
}

- (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
Expand Down
140 changes: 86 additions & 54 deletions src/darwin/Framework/CHIP/MTRCallbackBridgeBase_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,90 +32,122 @@
* communication with the device in question has been established, as far as we
* know.
*/
class MTRCallbackBridgeBase {
};

// TODO: ADD NS_ASSUME_NONNULL_BEGIN to this header. When that happens, note
// that in MTRActionBlock the two callback pointers are nonnull and the two
// arguments of ResponseHandler are both nullable.
// arguments of MTRResponseHandler are both nullable.

typedef void (^ResponseHandler)(id value, NSError * error);
typedef CHIP_ERROR (^MTRActionBlock)(chip::Messaging::ExchangeManager & exchangeManager, const chip::SessionHandle & session,
chip::Callback::Cancelable * success, chip::Callback::Cancelable * failure);
typedef CHIP_ERROR (^MTRLocalActionBlock)(chip::Callback::Cancelable * success, chip::Callback::Cancelable * failure);
typedef void (*DefaultFailureCallbackType)(void *, CHIP_ERROR);
typedef void (^MTRResponseHandler)(id value, NSError * error);
typedef void (*MTRErrorCallback)(void * context, CHIP_ERROR error);

template <class T> class MTRCallbackBridge {
/**
* The bridge will pass itself as the last argument to the action block.
*
* The action block must do one of three things:
*
* 1) Return an error.
* 2) Call the "successCb" callback, with the bridge passed to the block as the
* context, possibly asynchronously.
* 3) Call the "failureCb" callback, with the bridge passed to the block as the
* context, possibly asynchronously.
*
* For an MTRCallbackBridge that has keepAlive set to true, the success/failure
* callbacks may be called multiple times. If keepAlive is false, there must be
* no calls after the first one.
*/
template <typename SuccessCallback>
using MTRActionBlockT = CHIP_ERROR (^)(chip::Messaging::ExchangeManager & exchangeManager, const chip::SessionHandle & session,
SuccessCallback successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge);
template <typename SuccessCallback>
using MTRLocalActionBlockT = CHIP_ERROR (^)(SuccessCallback successCb, MTRErrorCallback failureCb);

template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
public:
using MTRActionBlock = MTRActionBlockT<T>;
using MTRLocalActionBlock = MTRLocalActionBlockT<T>;

/**
* Run the given MTRLocalActionBlock on the Matter thread, then handle
* converting the value produced by the success callback to the right type
* so it can be passed to a callback of the type we're templated over.
*
* Does not attempt to establish any sessions to devices. Must not be used
* with any action blocks that need a session.
* Construct a callback bridge, which can then have DispatcLocalAction() called
* on it.
*/
MTRCallbackBridge(dispatch_queue_t queue, ResponseHandler handler, MTRLocalActionBlock action, T OnSuccessFn, bool keepAlive)
MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, T OnSuccessFn, bool keepAlive)
: mQueue(queue)
, mHandler(handler)
, mKeepAlive(keepAlive)
, mSuccess(OnSuccessFn, this)
, mFailure(OnFailureFn, this)
, mSuccess(OnSuccessFn)
, mFailure(OnFailureFn)
{
LogRequestStart();

// For now keep sync dispatch here.
dispatch_sync(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{
CHIP_ERROR err = action(mSuccess.Cancel(), mFailure.Cancel());
if (err != CHIP_NO_ERROR) {
NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(),
chip::ErrorStr(err));

// Take the normal async error-reporting codepath. This will also
// handle cleaning us up properly.
OnFailureFn(this, err);
}
});
}

/**
* Run the given MTRActionBlock on the Matter thread, after getting a CASE
* session (possibly pre-existing) to the given node ID on the fabric
* represented by the given MTRDeviceController. On success, convert the
* success value to whatever type it needs to be to call the callback type
* we're templated over.
* Construct a callback bridge, which can then have DispatchAction() called
* on it.
*/
MTRCallbackBridge(dispatch_queue_t queue, chip::NodeId nodeID, MTRDeviceController * controller, ResponseHandler handler,
MTRActionBlock action, T OnSuccessFn, bool keepAlive)
MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, MTRActionBlock action, T OnSuccessFn, bool keepAlive)
: mQueue(queue)
, mHandler(handler)
, mAction(action)
, mKeepAlive(keepAlive)
, mSuccess(OnSuccessFn, this)
, mFailure(OnFailureFn, this)
, mSuccess(OnSuccessFn)
, mFailure(OnFailureFn)
{
ActionWithNodeID(nodeID, controller);
}

/**
* Run the given MTRActionBlock on the Matter thread, after getting a CASE
* session (possibly pre-existing) to the given node ID on the fabric
* represented by the given MTRDeviceController. On success, convert the
* success value to whatever type it needs to be to call the callback type
* we're templated over. Once this function has been called, on a callback
* bridge allocated with `new`, the bridge object must not be accessed by
* the caller. The action block will handle deleting the bridge.
*/
void DispatchAction(chip::NodeId nodeID, MTRDeviceController * controller) && { ActionWithNodeID(nodeID, controller); }

/**
* Run the given MTRActionBlock on the Matter thread after getting a secure
* session corresponding to the given MTRBaseDevice. On success, convert
* the success value to whatever type it needs to be to call the callback
* type we're templated over.
* type we're templated over. Once this function has been called, on a callback
* bridge allocated with `new`, the bridge object must not be accessed by
* the caller. The action block will handle deleting the bridge.
*/
MTRCallbackBridge(dispatch_queue_t queue, MTRBaseDevice * device, ResponseHandler handler, MTRActionBlock action, T OnSuccessFn,
bool keepAlive)
: mQueue(queue)
, mHandler(handler)
, mAction(action)
, mKeepAlive(keepAlive)
, mSuccess(OnSuccessFn, this)
, mFailure(OnFailureFn, this)
void DispatchAction(MTRBaseDevice * device) &&
{
if (device.isPASEDevice) {
ActionWithPASEDevice(device);
} else {
ActionWithNodeID(device.nodeID, device.deviceController);
}
};
}

/**
* Run the given MTRLocalActionBlock on the Matter thread, then handle
* converting the value produced by the success callback to the right type
* so it can be passed to a callback of the type we're templated over.
*
* Does not attempt to establish any sessions to devices. Must not be used
* with any action blocks that need a session.
*/
void DispatchLocalAction(MTRLocalActionBlock action)
{
LogRequestStart();

// For now keep sync dispatch here.
dispatch_sync(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{
CHIP_ERROR err = action(mSuccess, mFailure);
if (err != CHIP_NO_ERROR) {
NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(),
chip::ErrorStr(err));

// Take the normal async error-reporting codepath. This will also
// handle cleaning us up properly.
OnFailureFn(this, err);
}
});
}

void ActionWithPASEDevice(MTRBaseDevice * device)
{
Expand Down Expand Up @@ -167,7 +199,7 @@ template <class T> class MTRCallbackBridge {
return;
}

CHIP_ERROR err = action(*exchangeManager, session.Value(), mSuccess.Cancel(), mFailure.Cancel());
CHIP_ERROR err = action(*exchangeManager, session.Value(), mSuccess, mFailure, this);
if (err != CHIP_NO_ERROR) {
NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(),
chip::ErrorStr(err));
Expand Down Expand Up @@ -218,12 +250,12 @@ template <class T> class MTRCallbackBridge {
});
}

ResponseHandler mHandler;
MTRResponseHandler mHandler;
MTRActionBlock mAction;
bool mKeepAlive;

chip::Callback::Callback<T> mSuccess;
chip::Callback::Callback<DefaultFailureCallbackType> mFailure;
T mSuccess;
MTRErrorCallback mFailure;

// Measure the time it took for the callback to trigger
NSDate * mRequestTime;
Expand Down
Loading

0 comments on commit 1ff789e

Please sign in to comment.