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

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.

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;

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

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

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)