From 903639c3287df7235537547d947cbbaf8da01feb Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Mon, 28 Nov 2022 08:51:36 -0800 Subject: [PATCH] [ObjC] Put out of range closed enum extension values in unknown fields. For normal fields, closed enums get their out of range values put into unknown fields, but that wasn't happening for extension fields, this fixes that by adding the validation during parsing. Also document on the getExtension API what happens with enums. Add tests to confirm expected behaviors. PiperOrigin-RevId: 491356730 --- objectivec/GPBMessage.h | 6 +++ objectivec/GPBMessage.m | 42 ++++++++++++----- objectivec/Tests/GPBMessageTests.m | 76 ++++++++++++++++++++++++++++++ 3 files changed, 113 insertions(+), 11 deletions(-) diff --git a/objectivec/GPBMessage.h b/objectivec/GPBMessage.h index 896a4c6577db..5b7ce0cb4a49 100644 --- a/objectivec/GPBMessage.h +++ b/objectivec/GPBMessage.h @@ -410,6 +410,12 @@ CF_EXTERN_C_END * repeated fields. If the extension is a Message one will be auto created for * you and returned similar to fields. * + * NOTE: For Enum extensions, if the enum was _closed_, then unknown values + * were parsed into the message's unknown fields instead of ending up in the + * extensions, just like what happens with singular/repeated fields. For open + * enums, the _raw_ value will be in the NSNumber, meaning if one does a + * `switch` on the values, a `default` case should also be included. + * * @param extension The extension descriptor of the extension to fetch. * * @return The extension matching the given descriptor, or nil if none found. diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m index b2d5b79a66a1..334217919c1e 100644 --- a/objectivec/GPBMessage.m +++ b/objectivec/GPBMessage.m @@ -108,6 +108,7 @@ static id CreateMapForField(GPBFieldDescriptor *field, GPBMessage *autocreator) static id GetMapIvarWithField(GPBMessage *self, GPBFieldDescriptor *field); static NSMutableDictionary *CloneExtensionMap(NSDictionary *extensionMap, NSZone *zone) __attribute__((ns_returns_retained)); +static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self); #ifdef DEBUG static NSError *MessageError(NSInteger code, NSDictionary *userInfo) { @@ -638,6 +639,7 @@ static id GetMapIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) { #endif // !defined(__clang_analyzer__) static id NewSingleValueFromInputStream(GPBExtensionDescriptor *extension, + GPBMessage *messageToGetExtension, GPBCodedInputStream *input, id extensionRegistry, GPBMessage *existingValue) @@ -645,6 +647,7 @@ static id NewSingleValueFromInputStream(GPBExtensionDescriptor *extension, // Note that this returns a retained value intentionally. static id NewSingleValueFromInputStream(GPBExtensionDescriptor *extension, + GPBMessage *messageToGetExtension, GPBCodedInputStream *input, id extensionRegistry, GPBMessage *existingValue) { @@ -681,8 +684,20 @@ static id NewSingleValueFromInputStream(GPBExtensionDescriptor *extension, return GPBCodedInputStreamReadRetainedBytes(state); case GPBDataTypeString: return GPBCodedInputStreamReadRetainedString(state); - case GPBDataTypeEnum: - return [[NSNumber alloc] initWithInt:GPBCodedInputStreamReadEnum(state)]; + case GPBDataTypeEnum: { + int32_t val = GPBCodedInputStreamReadEnum(&input->state_); + GPBEnumDescriptor *enumDescriptor = extension.enumDescriptor; + // If run with source generated before the closed enum support, all enums + // will be considers not closed, so casing to the enum type for a switch + // could cause things to fall off the end of a switch. + if (!enumDescriptor.isClosed || enumDescriptor.enumVerifier(val)) { + return [[NSNumber alloc] initWithInt:val]; + } else { + GPBUnknownFieldSet *unknownFields = GetOrMakeUnknownFields(messageToGetExtension); + [unknownFields mergeVarintField:extension->description_->fieldNumber value:val]; + return nil; + } + } case GPBDataTypeGroup: case GPBDataTypeMessage: { GPBMessage *message; @@ -726,9 +741,11 @@ static void ExtensionMergeFromInputStream(GPBExtensionDescriptor *extension, BOO int32_t length = GPBCodedInputStreamReadInt32(state); size_t limit = GPBCodedInputStreamPushLimit(state, length); while (GPBCodedInputStreamBytesUntilLimit(state) > 0) { - id value = NewSingleValueFromInputStream(extension, input, extensionRegistry, nil); - [message addExtension:extension value:value]; - [value release]; + id value = NewSingleValueFromInputStream(extension, message, input, extensionRegistry, nil); + if (value) { + [message addExtension:extension value:value]; + [value release]; + } } GPBCodedInputStreamPopLimit(state, limit); } else { @@ -737,13 +754,16 @@ static void ExtensionMergeFromInputStream(GPBExtensionDescriptor *extension, BOO if (!isRepeated && GPBDataTypeIsMessage(description->dataType)) { existingValue = [message getExistingExtension:extension]; } - id value = NewSingleValueFromInputStream(extension, input, extensionRegistry, existingValue); - if (isRepeated) { - [message addExtension:extension value:value]; - } else { - [message setExtension:extension value:value]; + id value = + NewSingleValueFromInputStream(extension, message, input, extensionRegistry, existingValue); + if (value) { + if (isRepeated) { + [message addExtension:extension value:value]; + } else { + [message setExtension:extension value:value]; + } + [value release]; } - [value release]; } } diff --git a/objectivec/Tests/GPBMessageTests.m b/objectivec/Tests/GPBMessageTests.m index 7d4cf79c3df2..6a1d48beda4b 100644 --- a/objectivec/Tests/GPBMessageTests.m +++ b/objectivec/Tests/GPBMessageTests.m @@ -1481,6 +1481,82 @@ - (void)testExtensionsMergeFrom { XCTAssertEqualObjects(message, message2); } +- (void)testClosedEnumsInExtensions { + // Only unknown values. + + NSData *data = + DataFromCStr("\xA8\x01\x0A" // optional_nested_enum_extension set to 10 + "\x98\x03\x0B" // repeated_nested_enum_extension set to 11 + "\xA2\x03\x01\x0C" // repeated_foreign_enum_extension set to 12 (packed) + ); + NSError *error = nil; + + TestAllExtensions *msg = [TestAllExtensions parseFromData:data + extensionRegistry:[self extensionRegistry] + error:&error]; + XCTAssertNil(error); + + XCTAssertFalse([msg hasExtension:[UnittestRoot optionalNestedEnumExtension]]); + XCTAssertFalse([msg hasExtension:[UnittestRoot repeatedNestedEnumExtension]]); + XCTAssertFalse([msg hasExtension:[UnittestRoot repeatedForeignEnumExtension]]); + + GPBUnknownFieldSet *unknownFields = msg.unknownFields; + GPBUnknownField *field = + [unknownFields getField:[UnittestRoot optionalNestedEnumExtension].fieldNumber]; + XCTAssertNotNil(field); + XCTAssertEqual(field.varintList.count, 1); + XCTAssertEqual([field.varintList valueAtIndex:0], 10); + field = [unknownFields getField:[UnittestRoot repeatedNestedEnumExtension].fieldNumber]; + XCTAssertNotNil(field); + XCTAssertEqual(field.varintList.count, 1); + XCTAssertEqual([field.varintList valueAtIndex:0], 11); + field = [unknownFields getField:[UnittestRoot repeatedForeignEnumExtension].fieldNumber]; + XCTAssertNotNil(field); + XCTAssertEqual(field.varintList.count, 1); + XCTAssertEqual([field.varintList valueAtIndex:0], 12); + + // Unknown and known, the known come though an unknown go to unknown fields. + + data = DataFromCStr( + "\xA8\x01\x01" // optional_nested_enum_extension set to 1 + "\xA8\x01\x0A" // optional_nested_enum_extension set to 10 + "\xA8\x01\x02" // optional_nested_enum_extension set to 2 + "\x98\x03\x02" // repeated_nested_enum_extension set to 2 + "\x98\x03\x0B" // repeated_nested_enum_extension set to 11 + "\x98\x03\x03" // repeated_nested_enum_extension set to 3 + "\xA2\x03\x03\x04\x0C\x06" // repeated_foreign_enum_extension set to 4, 12, 6 (packed) + ); + error = nil; + + msg = [TestAllExtensions parseFromData:data + extensionRegistry:[self extensionRegistry] + error:&error]; + XCTAssertNil(error); + + XCTAssertTrue([msg hasExtension:[UnittestRoot optionalNestedEnumExtension]]); + XCTAssertEqualObjects([msg getExtension:[UnittestRoot optionalNestedEnumExtension]], @2); + XCTAssertTrue([msg hasExtension:[UnittestRoot repeatedNestedEnumExtension]]); + id expected = @[ @2, @3 ]; + XCTAssertEqualObjects([msg getExtension:[UnittestRoot repeatedNestedEnumExtension]], expected); + XCTAssertTrue([msg hasExtension:[UnittestRoot repeatedForeignEnumExtension]]); + expected = @[ @4, @6 ]; + XCTAssertEqualObjects([msg getExtension:[UnittestRoot repeatedForeignEnumExtension]], expected); + + unknownFields = msg.unknownFields; + field = [unknownFields getField:[UnittestRoot optionalNestedEnumExtension].fieldNumber]; + XCTAssertNotNil(field); + XCTAssertEqual(field.varintList.count, 1); + XCTAssertEqual([field.varintList valueAtIndex:0], 10); + field = [unknownFields getField:[UnittestRoot repeatedNestedEnumExtension].fieldNumber]; + XCTAssertNotNil(field); + XCTAssertEqual(field.varintList.count, 1); + XCTAssertEqual([field.varintList valueAtIndex:0], 11); + field = [unknownFields getField:[UnittestRoot repeatedForeignEnumExtension].fieldNumber]; + XCTAssertNotNil(field); + XCTAssertEqual(field.varintList.count, 1); + XCTAssertEqual([field.varintList valueAtIndex:0], 12); +} + - (void)testDefaultingExtensionMessages { TestAllExtensions *message = [TestAllExtensions message];