diff options
author | Sergio Campamá <kaipi@google.com> | 2016-06-02 11:14:26 -0700 |
---|---|---|
committer | Thomas Van Lenten <thomasvl@google.com> | 2016-06-02 14:14:26 -0400 |
commit | e34c09182ebe0ed2958aefa809159498923743d3 (patch) | |
tree | 2eaf97f34c4ece1c6391fb795b20acc2d46fa145 /objectivec | |
parent | 0ab7a7f74458be56bfc65a71a46711637ee5962b (diff) | |
download | protobuf-e34c09182ebe0ed2958aefa809159498923743d3.tar.gz protobuf-e34c09182ebe0ed2958aefa809159498923743d3.tar.bz2 protobuf-e34c09182ebe0ed2958aefa809159498923743d3.zip |
Improving the granularity parsing errors (#1623)
Add more context to GPBCodedInputStream failures.
Have GPBMessage parsing apis extract out the GPBCodedInputStream information and expose it.
Update HeaderDocs with pointers to all error domains/codes.
Expand the unittests to cover the full set of errors reported.
Fixes https://github.com/google/protobuf/issues/1618
Diffstat (limited to 'objectivec')
-rw-r--r-- | objectivec/GPBCodedInputStream.h | 33 | ||||
-rw-r--r-- | objectivec/GPBCodedInputStream.m | 65 | ||||
-rw-r--r-- | objectivec/GPBMessage.h | 28 | ||||
-rw-r--r-- | objectivec/GPBMessage.m | 38 | ||||
-rw-r--r-- | objectivec/Tests/GPBMessageTests+Serialization.m | 101 |
5 files changed, 223 insertions, 42 deletions
diff --git a/objectivec/GPBCodedInputStream.h b/objectivec/GPBCodedInputStream.h index 44f69906..df9d97ba 100644 --- a/objectivec/GPBCodedInputStream.h +++ b/objectivec/GPBCodedInputStream.h @@ -35,6 +35,39 @@ NS_ASSUME_NONNULL_BEGIN +CF_EXTERN_C_BEGIN + +/// GPBCodedInputStream exception name. Exceptions raised from +/// GPBCodedInputStream contain an underlying error in the userInfo dictionary +/// under the GPBCodedInputStreamUnderlyingErrorKey key. +extern NSString *const GPBCodedInputStreamException; + +/// The key under which the underlying NSError from the exception is stored. +extern NSString *const GPBCodedInputStreamUnderlyingErrorKey; + +/// NSError domain used for GPBCodedInputStream errors. +extern NSString *const GPBCodedInputStreamErrorDomain; + +/// Error code for NSError with GPBCodedInputStreamErrorDomain. +typedef NS_ENUM(NSInteger, GPBCodedInputStreamErrorCode) { + /// The size does not fit in the remaining bytes to be read. + GPBCodedInputStreamErrorInvalidSize = -100, + /// Attempted to read beyond the subsection limit. + GPBCodedInputStreamErrorSubsectionLimitReached = -101, + /// The requested subsection limit is invalid. + GPBCodedInputStreamErrorInvalidSubsectionLimit = -102, + /// Invalid tag read. + GPBCodedInputStreamErrorInvalidTag = -103, + /// Invalid UTF-8 character in a string. + GPBCodedInputStreamErrorInvalidUTF8 = -104, + /// Invalid VarInt read. + GPBCodedInputStreamErrorInvalidVarInt = -105, + /// The maximum recursion depth of messages was exceeded. + GPBCodedInputStreamErrorRecursionDepthExceeded = -106, +}; + +CF_EXTERN_C_END + /// Reads and decodes protocol message fields. /// /// The common uses of protocol buffers shouldn't need to use this class. diff --git a/objectivec/GPBCodedInputStream.m b/objectivec/GPBCodedInputStream.m index 40227178..acba7156 100644 --- a/objectivec/GPBCodedInputStream.m +++ b/objectivec/GPBCodedInputStream.m @@ -36,17 +36,42 @@ #import "GPBUtilities_PackagePrivate.h" #import "GPBWireFormat.h" +NSString *const GPBCodedInputStreamException = + GPBNSStringifySymbol(GPBCodedInputStreamException); + +NSString *const GPBCodedInputStreamUnderlyingErrorKey = + GPBNSStringifySymbol(GPBCodedInputStreamUnderlyingErrorKey); + +NSString *const GPBCodedInputStreamErrorDomain = + GPBNSStringifySymbol(GPBCodedInputStreamErrorDomain); + static const NSUInteger kDefaultRecursionLimit = 64; +static void RaiseException(NSInteger code, NSString *reason) { + NSDictionary *errorInfo = nil; + if ([reason length]) { + errorInfo = @{ GPBErrorReasonKey: reason }; + } + NSError *error = [NSError errorWithDomain:GPBCodedInputStreamErrorDomain + code:code + userInfo:errorInfo]; + + NSDictionary *exceptionInfo = + @{ GPBCodedInputStreamUnderlyingErrorKey: error }; + [[[NSException alloc] initWithName:GPBCodedInputStreamException + reason:reason + userInfo:exceptionInfo] raise]; +} + static void CheckSize(GPBCodedInputStreamState *state, size_t size) { size_t newSize = state->bufferPos + size; if (newSize > state->bufferSize) { - [NSException raise:NSParseErrorException format:@""]; + RaiseException(GPBCodedInputStreamErrorInvalidSize, nil); } if (newSize > state->currentLimit) { // Fast forward to end of currentLimit; state->bufferPos = state->currentLimit; - [NSException raise:NSParseErrorException format:@""]; + RaiseException(GPBCodedInputStreamErrorSubsectionLimitReached, nil); } } @@ -95,8 +120,8 @@ static int32_t ReadRawVarint32(GPBCodedInputStreamState *state) { return result; } } - [NSException raise:NSParseErrorException - format:@"Unable to read varint32"]; + RaiseException(GPBCodedInputStreamErrorInvalidVarInt, + @"Invalid VarInt32"); } } } @@ -115,7 +140,7 @@ static int64_t ReadRawVarint64(GPBCodedInputStreamState *state) { } shift += 7; } - [NSException raise:NSParseErrorException format:@"Unable to read varint64"]; + RaiseException(GPBCodedInputStreamErrorInvalidVarInt, @"Invalid VarInt64"); return 0; } @@ -202,8 +227,7 @@ int32_t GPBCodedInputStreamReadTag(GPBCodedInputStreamState *state) { state->lastTag = ReadRawVarint32(state); if (state->lastTag == 0) { // If we actually read zero, that's not a valid tag. - [NSException raise:NSParseErrorException - format:@"Invalid last tag %d", state->lastTag]; + RaiseException(GPBCodedInputStreamErrorInvalidTag, @"Last tag can't be 0"); } return state->lastTag; } @@ -226,8 +250,7 @@ NSString *GPBCodedInputStreamReadRetainedString( NSLog(@"UTF-8 failure, is some field type 'string' when it should be " @"'bytes'?"); #endif - [NSException raise:NSParseErrorException - format:@"Invalid UTF-8 for a 'string'"]; + RaiseException(GPBCodedInputStreamErrorInvalidUTF8, nil); } } return result; @@ -262,8 +285,7 @@ size_t GPBCodedInputStreamPushLimit(GPBCodedInputStreamState *state, byteLimit += state->bufferPos; size_t oldLimit = state->currentLimit; if (byteLimit > oldLimit) { - [NSException raise:NSInvalidArgumentException - format:@"byteLimit > oldLimit: %tu > %tu", byteLimit, oldLimit]; + RaiseException(GPBCodedInputStreamErrorInvalidSubsectionLimit, nil); } state->currentLimit = byteLimit; return oldLimit; @@ -286,8 +308,7 @@ BOOL GPBCodedInputStreamIsAtEnd(GPBCodedInputStreamState *state) { void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state, int32_t value) { if (state->lastTag != value) { - [NSException raise:NSParseErrorException - format:@"Last tag: %d should be %d", state->lastTag, value]; + RaiseException(GPBCodedInputStreamErrorInvalidTag, @"Unexpected tag read"); } } @@ -353,7 +374,7 @@ void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state, SkipRawData(&state_, sizeof(int32_t)); return YES; } - [NSException raise:NSParseErrorException format:@"Invalid tag %d", tag]; + RaiseException(GPBCodedInputStreamErrorInvalidTag, nil); return NO; } @@ -414,9 +435,7 @@ void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state, message:(GPBMessage *)message extensionRegistry:(GPBExtensionRegistry *)extensionRegistry { if (state_.recursionDepth >= kDefaultRecursionLimit) { - [NSException raise:NSParseErrorException - format:@"recursionDepth(%tu) >= %tu", state_.recursionDepth, - kDefaultRecursionLimit]; + RaiseException(GPBCodedInputStreamErrorRecursionDepthExceeded, nil); } ++state_.recursionDepth; [message mergeFromCodedInputStream:self extensionRegistry:extensionRegistry]; @@ -428,9 +447,7 @@ void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state, - (void)readUnknownGroup:(int32_t)fieldNumber message:(GPBUnknownFieldSet *)message { if (state_.recursionDepth >= kDefaultRecursionLimit) { - [NSException raise:NSParseErrorException - format:@"recursionDepth(%tu) >= %tu", state_.recursionDepth, - kDefaultRecursionLimit]; + RaiseException(GPBCodedInputStreamErrorRecursionDepthExceeded, nil); } ++state_.recursionDepth; [message mergeFromCodedInputStream:self]; @@ -443,9 +460,7 @@ void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state, extensionRegistry:(GPBExtensionRegistry *)extensionRegistry { int32_t length = ReadRawVarint32(&state_); if (state_.recursionDepth >= kDefaultRecursionLimit) { - [NSException raise:NSParseErrorException - format:@"recursionDepth(%tu) >= %tu", state_.recursionDepth, - kDefaultRecursionLimit]; + RaiseException(GPBCodedInputStreamErrorRecursionDepthExceeded, nil); } size_t oldLimit = GPBCodedInputStreamPushLimit(&state_, length); ++state_.recursionDepth; @@ -461,9 +476,7 @@ void GPBCodedInputStreamCheckLastTagWas(GPBCodedInputStreamState *state, parentMessage:(GPBMessage *)parentMessage { int32_t length = ReadRawVarint32(&state_); if (state_.recursionDepth >= kDefaultRecursionLimit) { - [NSException raise:NSParseErrorException - format:@"recursionDepth(%tu) >= %tu", state_.recursionDepth, - kDefaultRecursionLimit]; + RaiseException(GPBCodedInputStreamErrorRecursionDepthExceeded, nil); } size_t oldLimit = GPBCodedInputStreamPushLimit(&state_, length); ++state_.recursionDepth; diff --git a/objectivec/GPBMessage.h b/objectivec/GPBMessage.h index 2249829f..b3b07793 100644 --- a/objectivec/GPBMessage.h +++ b/objectivec/GPBMessage.h @@ -49,12 +49,15 @@ extern NSString *const GPBMessageErrorDomain; /// Error code for NSError with GPBMessageErrorDomain. typedef NS_ENUM(NSInteger, GPBMessageErrorCode) { - /// The data being parsed is bad and a message can not be created from it. - GPBMessageErrorCodeMalformedData = -100, + /// Uncategorized error. + GPBMessageErrorCodeOther = -100, /// A message can't be serialized because it is missing required fields. GPBMessageErrorCodeMissingRequiredField = -101, }; +/// Key under which the error's reason is stored inside the userInfo dictionary. +extern NSString *const GPBErrorReasonKey; + CF_EXTERN_C_END /// Base class for all of the generated message classes. @@ -86,6 +89,9 @@ CF_EXTERN_C_END /// @note In DEBUG builds, the parsed message is checked to be sure all required /// fields were provided, and the parse will fail if some are missing. /// +/// @note The errors returned are likely coming from the domain and codes listed +/// at the top of this file and GPBCodedInputStream.h. +/// /// @param data The data to parse. /// @param errorPtr An optional error pointer to fill in with a failure reason if /// the data can not be parsed. @@ -101,6 +107,9 @@ CF_EXTERN_C_END /// @note In DEBUG builds, the parsed message is checked to be sure all required /// fields were provided, and the parse will fail if some are missing. /// +/// @note The errors returned are likely coming from the domain and codes listed +/// at the top of this file and GPBCodedInputStream.h. +/// /// @param data The data to parse. /// @param extensionRegistry The extension registry to use to look up extensions. /// @param errorPtr An optional error pointer to fill in with a failure @@ -119,6 +128,9 @@ CF_EXTERN_C_END /// @note In DEBUG builds, the parsed message is checked to be sure all required /// fields were provided, and the parse will fail if some are missing. /// +/// @note The errors returned are likely coming from the domain and codes listed +/// at the top of this file and GPBCodedInputStream.h. +/// /// @param input The stream to read data from. /// @param extensionRegistry The extension registry to use to look up extensions. /// @param errorPtr An optional error pointer to fill in with a failure @@ -139,6 +151,9 @@ CF_EXTERN_C_END /// the required fields are set. So this method can be used to reload /// messages that may not be complete. /// +/// @note The errors returned are likely coming from the domain and codes listed +/// at the top of this file and GPBCodedInputStream.h. +/// /// @param input The stream to read data from. /// @param extensionRegistry The extension registry to use to look up extensions. /// @param errorPtr An optional error pointer to fill in with a failure @@ -158,6 +173,9 @@ CF_EXTERN_C_END /// @note In DEBUG builds, the parsed message is checked to be sure all required /// fields were provided, and the parse will fail if some are missing. /// +/// @note The errors returned are likely coming from the domain and codes listed +/// at the top of this file and GPBCodedInputStream.h. +/// /// @param data The data to parse. /// @param errorPtr An optional error pointer to fill in with a failure reason if /// the data can not be parsed. @@ -171,6 +189,9 @@ CF_EXTERN_C_END /// @note In DEBUG builds, the parsed message is checked to be sure all required /// fields were provided, and the parse will fail if some are missing. /// +/// @note The errors returned are likely coming from the domain and codes listed +/// at the top of this file and GPBCodedInputStream.h. +/// /// @param data The data to parse. /// @param extensionRegistry The extension registry to use to look up extensions. /// @param errorPtr An optional error pointer to fill in with a failure @@ -188,6 +209,9 @@ CF_EXTERN_C_END /// the required fields are set. So this method can be used to reload /// messages that may not be complete. /// +/// @note The errors returned are likely coming from the domain and codes listed +/// at the top of this file and GPBCodedInputStream.h. +/// /// @param input The stream to read data from. /// @param extensionRegistry The extension registry to use to look up extensions. /// @param errorPtr An optional error pointer to fill in with a failure diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m index f1cb3927..919fb931 100644 --- a/objectivec/GPBMessage.m +++ b/objectivec/GPBMessage.m @@ -53,6 +53,8 @@ NSString *const GPBMessageErrorDomain = GPBNSStringifySymbol(GPBMessageErrorDomain); +NSString *const GPBErrorReasonKey = @"Reason"; + static NSString *const kGPBDataCoderKey = @"GPBData"; // @@ -96,20 +98,35 @@ static NSMutableDictionary *CloneExtensionMap(NSDictionary *extensionMap, NSZone *zone) __attribute__((ns_returns_retained)); +#ifdef DEBUG static NSError *MessageError(NSInteger code, NSDictionary *userInfo) { return [NSError errorWithDomain:GPBMessageErrorDomain code:code userInfo:userInfo]; } +#endif -static NSError *MessageErrorWithReason(NSInteger code, NSString *reason) { - NSDictionary *userInfo = nil; - if ([reason length]) { - userInfo = @{ @"Reason" : reason }; +static NSError *ErrorFromException(NSException *exception) { + NSError *error = nil; + + if ([exception.name isEqual:GPBCodedInputStreamException]) { + NSDictionary *exceptionInfo = exception.userInfo; + error = exceptionInfo[GPBCodedInputStreamUnderlyingErrorKey]; } - return MessageError(code, userInfo); -} + if (!error) { + NSString *reason = exception.reason; + NSDictionary *userInfo = nil; + if ([reason length]) { + userInfo = @{ GPBErrorReasonKey : reason }; + } + + error = [NSError errorWithDomain:GPBMessageErrorDomain + code:GPBMessageErrorCodeOther + userInfo:userInfo]; + } + return error; +} static void CheckExtension(GPBMessage *self, GPBExtensionDescriptor *extension) { @@ -817,8 +834,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) { [self release]; self = nil; if (errorPtr) { - *errorPtr = MessageErrorWithReason(GPBMessageErrorCodeMalformedData, - exception.reason); + *errorPtr = ErrorFromException(exception); } } #ifdef DEBUG @@ -849,8 +865,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) { [self release]; self = nil; if (errorPtr) { - *errorPtr = MessageErrorWithReason(GPBMessageErrorCodeMalformedData, - exception.reason); + *errorPtr = ErrorFromException(exception); } } #ifdef DEBUG @@ -1923,8 +1938,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) { @catch (NSException *exception) { message = nil; if (errorPtr) { - *errorPtr = MessageErrorWithReason(GPBMessageErrorCodeMalformedData, - exception.reason); + *errorPtr = ErrorFromException(exception); } } #ifdef DEBUG diff --git a/objectivec/Tests/GPBMessageTests+Serialization.m b/objectivec/Tests/GPBMessageTests+Serialization.m index 0d811a96..fad9773a 100644 --- a/objectivec/Tests/GPBMessageTests+Serialization.m +++ b/objectivec/Tests/GPBMessageTests+Serialization.m @@ -881,6 +881,103 @@ static NSData *DataFromCStr(const char *str) { XCTAssertEqualObjects(extsParse, extsOrig); } +- (void)testErrorSubsectionInvalidLimit { + NSData *data = DataFromCStr( + "\x0A\x08\x0A\x07\x12\x04\x72\x02\x4B\x50\x12\x04\x72\x02\x4B\x50"); + NSError *error = nil; + NestedTestAllTypes *msg = [NestedTestAllTypes parseFromData:data + error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorInvalidSubsectionLimit); +} + +- (void)testErrorSubsectionLimitReached { + NSData *data = DataFromCStr("\x0A\x06\x12\x03\x72\x02\x4B\x50"); + NSError *error = nil; + NestedTestAllTypes *msg = [NestedTestAllTypes parseFromData:data + error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorSubsectionLimitReached); +} + +- (void)testErrorInvalidVarint { + NSData *data = DataFromCStr("\x72\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF\xFF"); + NSError *error = nil; + TestAllTypes *msg = [TestAllTypes parseFromData:data error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorInvalidVarInt); +} + +- (void)testErrorInvalidUTF8 { + NSData *data = DataFromCStr("\x72\x04\xF4\xFF\xFF\xFF"); + NSError *error = nil; + TestAllTypes *msg = [TestAllTypes parseFromData:data error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorInvalidUTF8); +} + +- (void)testErrorInvalidSize { + NSData *data = DataFromCStr("\x72\x03\x4B\x50"); + NSError *error = nil; + NestedTestAllTypes *msg = [NestedTestAllTypes parseFromData:data + error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorInvalidSize); +} + +- (void)testErrorInvalidTag { + NSData *data = DataFromCStr("\x0F"); + NSError *error = nil; + NestedTestAllTypes *msg = [NestedTestAllTypes parseFromData:data + error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorInvalidTag); +} + +- (void)testErrorRecursionDepthReached { + NSData *data = DataFromCStr( + "\x0A\x86\x01\x0A\x83\x01\x0A\x80\x01\x0A\x7E\x0A\x7C\x0A\x7A\x0A\x78" + "\x0A\x76\x0A\x74\x0A\x72\x0A\x70\x0A\x6E\x0A\x6C\x0A\x6A\x0A\x68" + "\x0A\x66\x0A\x64\x0A\x62\x0A\x60\x0A\x5E\x0A\x5C\x0A\x5A\x0A\x58" + "\x0A\x56\x0A\x54\x0A\x52\x0A\x50\x0A\x4E\x0A\x4C\x0A\x4A\x0A\x48" + "\x0A\x46\x0A\x44\x0A\x42\x0A\x40\x0A\x3E\x0A\x3C\x0A\x3A\x0A\x38" + "\x0A\x36\x0A\x34\x0A\x32\x0A\x30\x0A\x2E\x0A\x2C\x0A\x2A\x0A\x28" + "\x0A\x26\x0A\x24\x0A\x22\x0A\x20\x0A\x1E\x0A\x1C\x0A\x1A\x0A\x18" + "\x0A\x16\x0A\x14\x0A\x12\x0A\x10\x0A\x0E\x0A\x0C\x0A\x0A\x0A\x08" + "\x0A\x06\x12\x04\x72\x02\x4B\x50"); + NSError *error = nil; + NestedTestAllTypes *msg = [NestedTestAllTypes parseFromData:data + error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorRecursionDepthExceeded); +} + +#ifdef DEBUG +- (void)testErrorMissingRequiredField { + NSData *data = DataFromCStr(""); + NSError *error = nil; + TestRequired *msg = [TestRequired parseFromData:data error:&error]; + XCTAssertNil(msg); + XCTAssertNotNil(error); + XCTAssertEqualObjects(error.domain, GPBMessageErrorDomain); + XCTAssertEqual(error.code, GPBMessageErrorCodeMissingRequiredField); +} +#endif + #pragma mark - Subset from from map_tests.cc // TEST(GeneratedMapFieldTest, StandardWireFormat) @@ -989,8 +1086,8 @@ static NSData *DataFromCStr(const char *str) { TestMap *msg = [TestMap parseFromData:data error:&error]; XCTAssertNil(msg); XCTAssertNotNil(error); - XCTAssertEqualObjects(error.domain, GPBMessageErrorDomain); - XCTAssertEqual(error.code, GPBMessageErrorCodeMalformedData); + XCTAssertEqualObjects(error.domain, GPBCodedInputStreamErrorDomain); + XCTAssertEqual(error.code, GPBCodedInputStreamErrorInvalidSubsectionLimit); } // TEST(GeneratedMapFieldTest, Proto2UnknownEnum) |