From d393274fc9cd098848c5d6c98c5bef5d922fa826 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 2 May 2023 20:39:45 -0400 Subject: [PATCH] Stop requiring a fixed-size up-front allocation for our attribute value. --- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 89 +++++++++++------ .../CHIPTests/MTRDataValueParserTests.m | 98 +++++++++++++++---- src/system/TLVPacketBufferBackingStore.cpp | 2 +- 3 files changed, 141 insertions(+), 48 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index 6b8b7dde746799..d39dd74c2d2819 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -2266,7 +2266,7 @@ static void LogStringAndReturnError(NSString * errorStr, MTRErrorCode errorCode, static void LogStringAndReturnError(NSString * errorStr, CHIP_ERROR errorCode, NSError * __autoreleasing * error) { - MTR_LOG_ERROR("%s", errorStr.UTF8String); + MTR_LOG_ERROR("%s: %s", errorStr.UTF8String, errorCode.AsString()); if (!error) { return; } @@ -2292,30 +2292,72 @@ static bool CheckMemberOfType(NSDictionary * responseValue, NSSt return true; } -// Allocates a buffer, encodes the data-value as TLV, and points the TLV::Reader to -// the data. Returns false if any of that fails, in which case error gets set. -static bool EncodeDataValueToTLV(Platform::ScopedMemoryBuffer & buffer, size_t bufferSize, NSDictionary * data, - TLV::TLVReader & reader, NSError * __autoreleasing * error) -{ - if (!buffer.Calloc(bufferSize)) { - LogStringAndReturnError(@"Unable to allocate encoding buffer.", CHIP_ERROR_NO_MEMORY, error); +// Allocates a buffer, encodes the data-value as TLV, and points the TLV::Reader +// to the data. Returns false if any of that fails, in which case error gets +// set. +// +// Data model decoding requires a contiguous buffer (because lists walk all the +// data multiple times and TLVPacketBufferBackingStore doesn't have a way to +// checkpoint and restore its state), but we can encode into chained packet +// buffers and then decide whether we need a contiguous realloc. +static bool EncodeDataValueToTLV(System::PacketBufferHandle & buffer, Platform::ScopedMemoryBuffer & flatBuffer, + NSDictionary * data, TLV::TLVReader & reader, NSError * __autoreleasing * error) +{ + buffer = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSizeWithoutReserve, 0); + if (buffer.IsNull()) { + LogStringAndReturnError(@"Unable to allocate encoding buffer", CHIP_ERROR_NO_MEMORY, error); return false; } - TLV::TLVWriter writer; - writer.Init(buffer.Get(), bufferSize); + System::PacketBufferTLVWriter writer; + writer.Init(std::move(buffer), /* useChainedBuffers = */ true); CHIP_ERROR errorCode = MTREncodeTLVFromDataValueDictionary(data, writer, TLV::AnonymousTag()); if (errorCode != CHIP_NO_ERROR) { - LogStringAndReturnError(@"Unable to encode data-value to TLV.", errorCode, error); + LogStringAndReturnError(@"Unable to encode data-value to TLV", errorCode, error); + return false; + } + + errorCode = writer.Finalize(&buffer); + if (errorCode != CHIP_NO_ERROR) { + LogStringAndReturnError(@"Unable to encode data-value to TLV", errorCode, error); return false; } - reader.Init(buffer.Get(), writer.GetLengthWritten()); + if (buffer->HasChainedBuffer()) { + // We need to reallocate into a single contiguous buffer. + size_t remainingData = buffer->TotalLength(); + if (!flatBuffer.Calloc(remainingData)) { + LogStringAndReturnError(@"Unable to allocate decoding buffer", CHIP_ERROR_NO_MEMORY, error); + return false; + } + size_t copiedData = 0; + while (!buffer.IsNull()) { + if (buffer->DataLength() > remainingData) { + // Should never happen, but let's be extra careful about buffer + // overruns. + LogStringAndReturnError(@"Encoding buffer size is bigger than it claimed", CHIP_ERROR_INCORRECT_STATE, error); + return false; + } + + memcpy(flatBuffer.Get() + copiedData, buffer->Start(), buffer->DataLength()); + copiedData += buffer->DataLength(); + remainingData -= buffer->DataLength(); + buffer.Advance(); + } + if (remainingData != 0) { + LogStringAndReturnError( + @"Did not copy all data from Encoding buffer for some reason", CHIP_ERROR_INCORRECT_STATE, error); + return false; + } + reader.Init(flatBuffer.Get(), copiedData); + } else { + reader.Init(buffer->Start(), buffer->DataLength()); + } errorCode = reader.Next(TLV::AnonymousTag()); if (errorCode != CHIP_NO_ERROR) { - LogStringAndReturnError(@"data-value TLV encoding did not create a TLV element.", errorCode, error); + LogStringAndReturnError(@"data-value TLV encoding did not create a TLV element", errorCode, error); return false; } @@ -2374,18 +2416,10 @@ - (nullable instancetype)initWithResponseValue:(NSDictionary *)r NSDictionary * data = responseValue[MTRDataKey]; // Encode the data to TLV and then decode from that, to reuse existing code. - // We don't know exactly how much data we can have here; if our value is a list it - // might well be larger than the amount of data that would fit in a single packet. - // - // We could start with some small buffer size and try growing it until it's - // big enough to encode into, but in practice we'd probably have to cap that - // at some max size anyway. So just start with something that is likely to - // work for any conceivable attribute, like 20KiB. - - constexpr size_t bufferSize = 20 * 1042; - Platform::ScopedMemoryBuffer buffer; + System::PacketBufferHandle buffer; + Platform::ScopedMemoryBuffer flatBuffer; TLV::TLVReader reader; - if (!EncodeDataValueToTLV(buffer, bufferSize, data, reader, error)) { + if (!EncodeDataValueToTLV(buffer, flatBuffer, data, reader, error)) { return nil; } @@ -2507,11 +2541,10 @@ - (nullable instancetype)initWithResponseValue:(NSDictionary *)r NSDictionary * data = responseValue[MTRDataKey]; // Encode the data to TLV and then decode from that, to reuse existing code. - // For an event, we know it always fits in a single Matter message. - constexpr size_t bufferSize = kMaxSecureSduLengthBytes; - Platform::ScopedMemoryBuffer buffer; + System::PacketBufferHandle buffer; + Platform::ScopedMemoryBuffer flatBuffer; TLV::TLVReader reader; - if (!EncodeDataValueToTLV(buffer, bufferSize, data, reader, error)) { + if (!EncodeDataValueToTLV(buffer, flatBuffer, data, reader, error)) { return nil; } auto eventPath = [path _asConcretePath]; diff --git a/src/darwin/Framework/CHIPTests/MTRDataValueParserTests.m b/src/darwin/Framework/CHIPTests/MTRDataValueParserTests.m index a12b20319fda9a..1eee9de6ccaded 100644 --- a/src/darwin/Framework/CHIPTests/MTRDataValueParserTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDataValueParserTests.m @@ -218,7 +218,7 @@ - (void)test009_NullableDoubleAttribute XCTAssertNil(report.error); } -- (void)test0010_StructAttribute +- (void)test010_StructAttribute { // Basic Information, CapabilityMinima NSDictionary * input = @{ @@ -260,7 +260,7 @@ - (void)test0010_StructAttribute XCTAssertNil(report.error); } -- (void)test0011_StructAttributeOtherOrder +- (void)test011_StructAttributeOtherOrder { // Basic Information, CapabilityMinima NSDictionary * input = @{ @@ -302,7 +302,7 @@ - (void)test0011_StructAttributeOtherOrder XCTAssertNil(report.error); } -- (void)test0012_ListAttribute +- (void)test012_ListAttribute { // Descriptor, DeviceTypeList NSDictionary * input = @{ @@ -379,7 +379,7 @@ - (void)test0012_ListAttribute XCTAssertNil(report.error); } -- (void)test0013_UnsignedIntAttributeSignMismatch +- (void)test013_UnsignedIntAttributeSignMismatch { // Pressure Measurement, Tolerance NSDictionary * input = @{ @@ -397,7 +397,7 @@ - (void)test0013_UnsignedIntAttributeSignMismatch XCTAssertEqual(error.code, MTRErrorCodeSchemaMismatch); } -- (void)test0014_SignedIntAttributeSignMismatch +- (void)test014_SignedIntAttributeSignMismatch { // Pressure Measurement, MeasuredValue NSDictionary * input = @{ @@ -415,7 +415,7 @@ - (void)test0014_SignedIntAttributeSignMismatch XCTAssertEqual(error.code, MTRErrorCodeSchemaMismatch); } -- (void)test0015_UnknownAttribute +- (void)test015_UnknownAttribute { // On/Off, nonexistent attribute NSDictionary * input = @{ @@ -433,7 +433,7 @@ - (void)test0015_UnknownAttribute XCTAssertEqual(error.code, MTRErrorCodeUnknownSchema); } -- (void)test0016_UnknownCluster +- (void)test016_UnknownCluster { // Unknown cluster. NSDictionary * input = @{ @@ -451,7 +451,7 @@ - (void)test0016_UnknownCluster XCTAssertEqual(error.code, MTRErrorCodeUnknownSchema); } -- (void)test0017_StringVsOctetStringMismatch +- (void)test017_StringVsOctetStringMismatch { // Basic Information, SerialNumber NSDictionary * input = @{ @@ -469,7 +469,7 @@ - (void)test0017_StringVsOctetStringMismatch XCTAssertEqual(error.code, MTRErrorCodeSchemaMismatch); } -- (void)test0018_OctetStringVsStringMismatch +- (void)test018_OctetStringVsStringMismatch { // Thread Network Diagnostics, ChannelPage0Mask NSDictionary * input = @{ @@ -487,7 +487,7 @@ - (void)test0018_OctetStringVsStringMismatch XCTAssertEqual(error.code, MTRErrorCodeSchemaMismatch); } -- (void)test0019_DoubleVsFloatMismatch +- (void)test019_DoubleVsFloatMismatch { // Unit Testing, float_double NSDictionary * input = @{ @@ -510,7 +510,7 @@ - (void)test0019_DoubleVsFloatMismatch XCTAssertNil(report.error); } -- (void)test0020_FloatVsDoubleMismatch +- (void)test020_FloatVsDoubleMismatch { // Media Playback, PlaybackSpeed NSDictionary * input = @{ @@ -579,7 +579,7 @@ - (void)test023_DoubleVsNullMismatch XCTAssertEqual(error.code, MTRErrorCodeSchemaMismatch); } -- (void)test0024_StructFieldIntegerTypeMismatch +- (void)test024_StructFieldIntegerTypeMismatch { // Descriptor, DeviceTypeList NSDictionary * input = @{ @@ -640,7 +640,7 @@ - (void)test0024_StructFieldIntegerTypeMismatch XCTAssertEqual(error.code, MTRErrorCodeSchemaMismatch); } -- (void)test0025_EventPayloadWithSystemUptime +- (void)test025_EventPayloadWithSystemUptime { // Access Control, AccessControlExtensionChanged NSDictionary * input = @{ @@ -722,7 +722,7 @@ - (void)test0025_EventPayloadWithSystemUptime XCTAssertNil(report.error); } -- (void)test0026_EventReportWithTimestampDate +- (void)test026_EventReportWithTimestampDate { // Basic Information, Shutdown NSDictionary * input = @{ @@ -756,7 +756,7 @@ - (void)test0026_EventReportWithTimestampDate XCTAssertTrue([report.value isKindOfClass:[MTRBasicInformationClusterShutDownEvent class]]); } -- (void)test0027_AttributeWithDataAndError +- (void)test027_AttributeWithDataAndError { // Pressure Measurement, Tolerance NSDictionary * input = @{ @@ -779,7 +779,7 @@ - (void)test0027_AttributeWithDataAndError XCTAssertEqualObjects(report.error, input[MTRErrorKey]); } -- (void)test0028_EventReportWithDataAndError +- (void)test028_EventReportWithDataAndError { // Basic Information, Shutdown NSDictionary * input = @{ @@ -813,7 +813,7 @@ - (void)test0028_EventReportWithDataAndError XCTAssertEqualObjects(report.error, input[MTRErrorKey]); } -- (void)test0029_EventPayloadFailingSchemaCheck +- (void)test029_EventPayloadFailingSchemaCheck { // Access Control, AccessControlExtensionChanged NSDictionary * input = @{ @@ -871,7 +871,7 @@ - (void)test0029_EventPayloadFailingSchemaCheck XCTAssertEqual(error.code, MTRErrorCodeSchemaMismatch); } -- (void)test0030_EventReportWithUnknownCluster +- (void)test030_EventReportWithUnknownCluster { NSDictionary * input = @{ MTREventPathKey : [MTREventPath eventPathWithEndpointID:@(0) clusterID:@(0xFF1FF1) eventID:@(0)], @@ -892,7 +892,7 @@ - (void)test0030_EventReportWithUnknownCluster XCTAssertEqual(error.code, MTRErrorCodeUnknownSchema); } -- (void)test0031_EventReportWithUnknownEvent +- (void)test031_EventReportWithUnknownEvent { NSDictionary * input = @{ MTREventPathKey : [MTREventPath eventPathWithEndpointID:@(0) clusterID:@(0x0028) eventID:@(1000)], @@ -913,4 +913,64 @@ - (void)test0031_EventReportWithUnknownEvent XCTAssertEqual(error.code, MTRErrorCodeUnknownSchema); } +- (void)test032_VeryLongListAttribute +{ + NSDictionary * singleListItem = @{ + MTRDataKey : @ { + MTRTypeKey : MTRStructureValueType, + MTRValueKey : @[ + @{ + MTRContextTagKey : @(0), // DeviceType + MTRDataKey : @ { + MTRTypeKey : MTRUnsignedIntegerValueType, + MTRValueKey : @(100), + }, + }, + @{ + MTRContextTagKey : @(1), // Revision + MTRDataKey : @ { + MTRTypeKey : MTRUnsignedIntegerValueType, + MTRValueKey : @(17), + }, + }, + ], + }, + }; + + NSUInteger arrayLength = 1000; + NSMutableArray * inputArray = [[NSMutableArray alloc] initWithCapacity:arrayLength]; + for (NSUInteger i = 0; i < arrayLength; ++i) { + [inputArray addObject:singleListItem]; + } + + // Descriptor, DeviceTypeList + NSDictionary * input = @{ + MTRAttributePathKey : [MTRAttributePath attributePathWithEndpointID:@(0) clusterID:@(0x001d) attributeID:@(0)], + MTRDataKey : @ { + MTRTypeKey : MTRArrayValueType, + MTRValueKey : inputArray, + }, + }; + + NSError * error; + __auto_type * report = [[MTRAttributeReport alloc] initWithResponseValue:input error:&error]; + XCTAssertNil(error); + XCTAssertNotNil(report); + + XCTAssertEqualObjects(input[MTRAttributePathKey], report.path); + XCTAssertNotNil(report.value); + XCTAssertTrue([report.value isKindOfClass:[NSArray class]]); + + NSArray * array = report.value; + XCTAssertTrue(array.count == inputArray.count); + for (id item in array) { + XCTAssertTrue([item isKindOfClass:[MTRDescriptorClusterDeviceTypeStruct class]]); + MTRDescriptorClusterDeviceTypeStruct * deviceType = item; + XCTAssertEqualObjects(deviceType.deviceType, @(100)); + XCTAssertEqualObjects(deviceType.revision, @(17)); + } + + XCTAssertNil(report.error); +} + @end diff --git a/src/system/TLVPacketBufferBackingStore.cpp b/src/system/TLVPacketBufferBackingStore.cpp index f7a0702755528f..5059f837ff69a3 100644 --- a/src/system/TLVPacketBufferBackingStore.cpp +++ b/src/system/TLVPacketBufferBackingStore.cpp @@ -76,7 +76,7 @@ CHIP_ERROR TLVPacketBufferBackingStore::FinalizeBuffer(chip::TLV::TLVWriter & wr { return CHIP_ERROR_INVALID_ARGUMENT; } - mCurrentBuffer->SetDataLength(static_cast(length)); + mCurrentBuffer->SetDataLength(static_cast(length), mHeadBuffer); return CHIP_NO_ERROR; }