From 3375871a93f8d366f1eca5b823e4337af3aecd8c Mon Sep 17 00:00:00 2001 From: Timur Pocheptsov Date: Fri, 4 Nov 2016 15:56:59 +0100 Subject: OSXBTPeripheralManager - fix characteristic write - Use request.value.bytes instead of data.bytes to write into ... data.bytes. - Report offset and invalid length on characteristic write as separate errors. - Check minimal/maximalValueLength. - Allow characteristic writes within min/maxValueLength. Task-number: QTBUG-56898 Change-Id: I624e55fe964e86b5bbf246ce2ee053dd247f7761 Reviewed-by: Alex Blasche --- src/bluetooth/osx/osxbtperipheralmanager.mm | 88 +++++++++++++++++++++++----- src/bluetooth/osx/osxbtperipheralmanager_p.h | 9 ++- 2 files changed, 80 insertions(+), 17 deletions(-) diff --git a/src/bluetooth/osx/osxbtperipheralmanager.mm b/src/bluetooth/osx/osxbtperipheralmanager.mm index 7a27b66b..a44fb95c 100644 --- a/src/bluetooth/osx/osxbtperipheralmanager.mm +++ b/src/bluetooth/osx/osxbtperipheralmanager.mm @@ -148,6 +148,16 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) return nEntries; } +bool qt_validate_value_range(const QLowEnergyCharacteristicData &data) +{ + if (data.minimumValueLength() > data.maximumValueLength() + || data.minimumValueLength() < 0) { + return false; + } + + return data.value().size() <= data.maximumValueLength(); +} + } @interface QT_MANGLE_NAMESPACE(OSXBTPeripheralManager) (PrivateAPI) @@ -336,14 +346,24 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) QT_BT_MAC_AUTORELEASEPOOL - if (!charMap.contains(charHandle)) { + if (!charMap.contains(charHandle) || !valueRanges.contains(charHandle)) { emit notifier->CBManagerError(QLowEnergyController::UnknownError); return; } + const auto & range = valueRanges[charHandle]; + if (value.size() < int(range.first) || value.size() > int(range.second)) { + qCWarning(QT_BT_OSX) << "ignoring value of invalid length" << value.count(); + return; + } + const auto nsData = mutable_data_from_bytearray(value); charValues[charHandle] = nsData; - updateQueue.push_back(UpdateRequest{charHandle, nsData}); + // We copy data here: sending update requests is async (see sendUpdateRequests), + // by the time we're allowed to actually send them, the data can change again + // and we'll send an 'out of order' value. + const ObjCStrongReference copy([NSData dataWithData:nsData], true); + updateQueue.push_back(UpdateRequest{charHandle, copy}); [self sendUpdateRequests]; } @@ -509,6 +529,27 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) [manager respondToRequest:request withResult:CBATTErrorSuccess]; } + +- (void)writeValueForCharacteristic:(QLowEnergyHandle) charHandle + withWriteRequest:(CBATTRequest *)request +{ + Q_ASSERT(charHandle); + Q_ASSERT(request); + + Q_ASSERT(valueRanges.contains(charHandle)); + const auto &range = valueRanges[charHandle]; + Q_ASSERT(request.offset <= range.second + && request.value.length <= range.second - request.offset); + + Q_ASSERT(charValues.contains(charHandle)); + NSMutableData *const value = charValues[charHandle]; + if (request.offset + request.value.length > value.length) + [value increaseLengthBy:request.offset + request.value.length - value.length]; + + [value replaceBytesInRange:NSMakeRange(request.offset, request.value.length) + withBytes:request.value.bytes]; +} + - (void)peripheralManager:(CBPeripheralManager *)peripheral didReceiveWriteRequests:(NSArray *)requests { @@ -539,18 +580,22 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) const auto charHandle = charMap.key(request.characteristic); updated.insert(charHandle); - NSMutableData *const data = charValues[charHandle]; - [data replaceBytesInRange:NSMakeRange(request.offset, request.value.length) - withBytes:data.bytes]; + [self writeValueForCharacteristic:charHandle withWriteRequest:request]; } - for (const auto handle : updated) + for (const auto handle : updated) { emit notifier->characteristicUpdated(handle, qt_bytearray(charValues[handle])); + const ObjCStrongReference copy([NSData dataWithData:charValues[handle]], + true); + updateQueue.push_back(UpdateRequest{handle, copy}); + } if (requests.count) { [manager respondToRequest:[requests objectAtIndex:0] withResult:CBATTErrorSuccess]; } + + [self sendUpdateRequests]; } - (void)peripheralManagerIsReadyToUpdateSubscribers:(CBPeripheralManager *)peripheral @@ -569,13 +614,14 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) while (updateQueue.size()) { const auto &request = updateQueue.front(); - Q_ASSERT(charMap.contains(request.charHandle)); - const BOOL res = [manager updateValue:request.value - forCharacteristic:static_cast(charMap[request.charHandle]) - onSubscribedCentrals:nil]; - if (!res) { - // Have to wait for the 'ManagerIsReadyToUpdate'. - break; + if (charMap.contains(request.charHandle)) { + const BOOL res = [manager updateValue:request.value + forCharacteristic:static_cast(charMap[request.charHandle]) + onSubscribedCentrals:nil]; + if (!res) { + // Have to wait for the 'ManagerIsReadyToUpdate'. + break; + } } updateQueue.pop_front(); @@ -682,6 +728,12 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) } for (const auto &ch : data.characteristics()) { + if (!qt_validate_value_range(ch)) { + qCWarning(QT_BT_OSX) << "addCharacteristicsAndDescritptors: " + "invalid value size/min-max length"; + continue; + } + const auto cbChar(create_characteristic(ch)); if (!cbChar) { qCWarning(QT_BT_OSX) << "addCharacteristicsAndDescritptors: " @@ -702,6 +754,7 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) // CB part: charMap[declHandle] = cbChar; charValues[declHandle] = nsData; + valueRanges[declHandle] = ValueRange(ch.minimumValueLength(), ch.maximumValueLength()); // QT part: QLowEnergyServicePrivate::CharData charData; charData.valueHandle = ++lastHandle; @@ -753,10 +806,15 @@ quint32 qt_countGATTEntries(const QLowEnergyServiceData &data) if (!handle || !charValues.contains(handle)) return CBATTErrorInvalidHandle; - NSMutableData *data = static_cast(charValues[handle]); - if (request.offset > data.length || request.value.length > data.length - request.offset) + Q_ASSERT(valueRanges.contains(handle)); + + const auto &range = valueRanges[handle]; + if (request.offset > range.second) return CBATTErrorInvalidOffset; + if (request.value.length > range.second - request.offset) + return CBATTErrorInvalidAttributeValueLength; + return CBATTErrorSuccess; } diff --git a/src/bluetooth/osx/osxbtperipheralmanager_p.h b/src/bluetooth/osx/osxbtperipheralmanager_p.h index 3367b367..b6021fb8 100644 --- a/src/bluetooth/osx/osxbtperipheralmanager_p.h +++ b/src/bluetooth/osx/osxbtperipheralmanager_p.h @@ -52,6 +52,7 @@ #include #include #include +#include #include #include @@ -98,16 +99,18 @@ enum class PeripheralState struct UpdateRequest { UpdateRequest() = default; - UpdateRequest(QLowEnergyHandle handle, const ObjCStrongReference &val) + UpdateRequest(QLowEnergyHandle handle, const ObjCStrongReference &val) : charHandle(handle), value(val) { } QLowEnergyHandle charHandle = {}; - ObjCStrongReference value; + ObjCStrongReference value; }; +using ValueRange = QPair; + @interface QT_MANGLE_NAMESPACE(OSXBTPeripheralManager) : NSObject { ObjCScopedPointer manager; @@ -127,6 +130,8 @@ struct UpdateRequest GenericLEMap charMap; GenericLEMap> charValues; + QMap valueRanges; + std::deque updateQueue; ObjCScopedPointer connectedCentrals; -- cgit v1.2.3