Improve number handling in JSON parsing

- Tighten up on Infinity/NaN handling in terms of whitespace handling (and test casing)
- Validate that values are genuinely integers when they've been parsed from a JSON number (ignoring the fact that 1.0000000000000000001 == 1 as a double...)
- Allow exponents and decimal points in string representations

1 files changed, 56 insertions, 39 deletions

diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs
index 95f9ad35..300a66b4 100644
--- a/csharp/src/Google.Protobuf/JsonParser.cs
+++ b/csharp/src/Google.Protobuf/JsonParser.cs
@@ -540,17 +540,17 @@ namespace Google.Protobuf
                 case FieldType.Int32:
                 case FieldType.SInt32:
                 case FieldType.SFixed32:
-                    return ParseNumericString(keyText, int.Parse, false);
+                    return ParseNumericString(keyText, int.Parse);
                 case FieldType.UInt32:
                 case FieldType.Fixed32:
-                    return ParseNumericString(keyText, uint.Parse, false);
+                    return ParseNumericString(keyText, uint.Parse);
                 case FieldType.Int64:
                 case FieldType.SInt64:
                 case FieldType.SFixed64:
-                    return ParseNumericString(keyText, long.Parse, false);
+                    return ParseNumericString(keyText, long.Parse);
                 case FieldType.UInt64:
                 case FieldType.Fixed64:
-                    return ParseNumericString(keyText, ulong.Parse, false);
+                    return ParseNumericString(keyText, ulong.Parse);
                 default:
                     throw new InvalidProtocolBufferException("Invalid field type for map: " + field.FieldType);
             }
@@ -561,7 +561,6 @@ namespace Google.Protobuf
             double value = token.NumberValue;
             checked
             {
-                // TODO: Validate that it's actually an integer, possibly in terms of the textual representation?                
                 try
                 {
                     switch (field.FieldType)
@@ -569,16 +568,20 @@ namespace Google.Protobuf
                         case FieldType.Int32:
                         case FieldType.SInt32:
                         case FieldType.SFixed32:
+                            CheckInteger(value);
                             return (int) value;
                         case FieldType.UInt32:
                         case FieldType.Fixed32:
+                            CheckInteger(value);
                             return (uint) value;
                         case FieldType.Int64:
                         case FieldType.SInt64:
                         case FieldType.SFixed64:
+                            CheckInteger(value);
                             return (long) value;
                         case FieldType.UInt64:
                         case FieldType.Fixed64:
+                            CheckInteger(value);
                             return (ulong) value;
                         case FieldType.Double:
                             return value;
@@ -597,20 +600,32 @@ namespace Google.Protobuf
                                 {
                                     return float.NegativeInfinity;
                                 }
-                                throw new InvalidProtocolBufferException("Value out of range: " + value);
+                                throw new InvalidProtocolBufferException($"Value out of range: {value}");
                             }
                             return (float) value;
                         default:
-                            throw new InvalidProtocolBufferException("Unsupported conversion from JSON number for field type " + field.FieldType);
+                            throw new InvalidProtocolBufferException($"Unsupported conversion from JSON number for field type {field.FieldType}");
                     }
                 }
                 catch (OverflowException)
                 {
-                    throw new InvalidProtocolBufferException("Value out of range: " + value);
+                    throw new InvalidProtocolBufferException($"Value out of range: {value}");
                 }
             }
         }
 
+        private static void CheckInteger(double value)
+        {
+            if (double.IsInfinity(value) || double.IsNaN(value))
+            {
+                throw new InvalidProtocolBufferException($"Value not an integer: {value}");
+            }
+            if (value != Math.Floor(value))
+            {
+                throw new InvalidProtocolBufferException($"Value not an integer: {value}");
+            }            
+        }
+
         private static object ParseSingleStringValue(FieldDescriptor field, string text)
         {
             switch (field.FieldType)
@@ -622,43 +637,35 @@ namespace Google.Protobuf
                 case FieldType.Int32:
                 case FieldType.SInt32:
                 case FieldType.SFixed32:
-                    return ParseNumericString(text, int.Parse, false);
+                    return ParseNumericString(text, int.Parse);
                 case FieldType.UInt32:
                 case FieldType.Fixed32:
-                    return ParseNumericString(text, uint.Parse, false);
+                    return ParseNumericString(text, uint.Parse);
                 case FieldType.Int64:
                 case FieldType.SInt64:
                 case FieldType.SFixed64:
-                    return ParseNumericString(text, long.Parse, false);
+                    return ParseNumericString(text, long.Parse);
                 case FieldType.UInt64:
                 case FieldType.Fixed64:
-                    return ParseNumericString(text, ulong.Parse, false);
+                    return ParseNumericString(text, ulong.Parse);
                 case FieldType.Double:
-                    double d = ParseNumericString(text, double.Parse, true);
-                    // double.Parse can return +/- infinity on Mono for non-infinite values which are out of range for double.
-                    if (double.IsInfinity(d) && !text.Contains("Infinity"))
-                    {
-                        throw new InvalidProtocolBufferException("Invalid numeric value: " + text);
-                    }
+                    double d = ParseNumericString(text, double.Parse);
+                    ValidateInfinityAndNan(text, double.IsPositiveInfinity(d), double.IsNegativeInfinity(d), double.IsNaN(d));
                     return d;
                 case FieldType.Float:
-                    float f = ParseNumericString(text, float.Parse, true);
-                    // float.Parse can return +/- infinity on Mono for non-infinite values which are out of range for float.
-                    if (float.IsInfinity(f) && !text.Contains("Infinity"))
-                    {
-                        throw new InvalidProtocolBufferException("Invalid numeric value: " + text);
-                    }
+                    float f = ParseNumericString(text, float.Parse);
+                    ValidateInfinityAndNan(text, float.IsPositiveInfinity(f), float.IsNegativeInfinity(f), float.IsNaN(f));
                     return f;
                 case FieldType.Enum:
                     var enumValue = field.EnumType.FindValueByName(text);
                     if (enumValue == null)
                     {
-                        throw new InvalidProtocolBufferException("Invalid enum value: " + text + " for enum type: " + field.EnumType.FullName);
+                        throw new InvalidProtocolBufferException($"Invalid enum value: {text} for enum type: {field.EnumType.FullName}");
                     }
                     // Just return it as an int, and let the CLR convert it.
                     return enumValue.Number;
                 default:
-                    throw new InvalidProtocolBufferException("Unsupported conversion from JSON string for field type " + field.FieldType);
+                    throw new InvalidProtocolBufferException($"Unsupported conversion from JSON string for field type {field.FieldType}");
             }
         }
 
@@ -670,43 +677,53 @@ namespace Google.Protobuf
             return field.MessageType.Parser.CreateTemplate();
         }
 
-        private static T ParseNumericString<T>(string text, Func<string, NumberStyles, IFormatProvider, T> parser, bool floatingPoint)
+        private static T ParseNumericString<T>(string text, Func<string, NumberStyles, IFormatProvider, T> parser)
         {
-            // TODO: Prohibit leading zeroes (but allow 0!)
-            // TODO: Validate handling of "Infinity" etc. (Should be case sensitive, no leading whitespace etc)
             // Can't prohibit this with NumberStyles.
             if (text.StartsWith("+"))
             {
-                throw new InvalidProtocolBufferException("Invalid numeric value: " + text);
+                throw new InvalidProtocolBufferException($"Invalid numeric value: {text}");
             }
             if (text.StartsWith("0") && text.Length > 1)
             {
                 if (text[1] >= '0' && text[1] <= '9')
                 {
-                    throw new InvalidProtocolBufferException("Invalid numeric value: " + text);
+                    throw new InvalidProtocolBufferException($"Invalid numeric value: {text}");
                 }
             }
             else if (text.StartsWith("-0") && text.Length > 2)
             {
                 if (text[2] >= '0' && text[2] <= '9')
                 {
-                    throw new InvalidProtocolBufferException("Invalid numeric value: " + text);
+                    throw new InvalidProtocolBufferException($"Invalid numeric value: {text}");
                 }
             }
             try
             {
-                var styles = floatingPoint
-                    ? NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent
-                    : NumberStyles.AllowLeadingSign;
-                return parser(text, styles, CultureInfo.InvariantCulture);
+                return parser(text, NumberStyles.AllowLeadingSign | NumberStyles.AllowDecimalPoint | NumberStyles.AllowExponent, CultureInfo.InvariantCulture);
             }
             catch (FormatException)
             {
-                throw new InvalidProtocolBufferException("Invalid numeric value for type: " + text);
+                throw new InvalidProtocolBufferException($"Invalid numeric value for type: {text}");
             }
             catch (OverflowException)
             {
-                throw new InvalidProtocolBufferException("Value out of range: " + text);
+                throw new InvalidProtocolBufferException($"Value out of range: {text}");
+            }
+        }
+
+        /// <summary>
+        /// Checks that any infinite/NaN values originated from the correct text.
+        /// This corrects the lenient whitespace handling of double.Parse/float.Parse, as well as the
+        /// way that Mono parses out-of-range values as infinity.
+        /// </summary>
+        private static void ValidateInfinityAndNan(string text, bool isPositiveInfinity, bool isNegativeInfinity, bool isNaN)
+        {
+            if ((isPositiveInfinity && text != "Infinity") ||
+                (isNegativeInfinity && text != "-Infinity") ||
+                (isNaN && text != "NaN"))
+            {
+                throw new InvalidProtocolBufferException($"Invalid numeric value: {text}");
             }
         }
 
@@ -719,7 +736,7 @@ namespace Google.Protobuf
             var match = TimestampRegex.Match(token.StringValue);
             if (!match.Success)
             {
-                throw new InvalidProtocolBufferException("Invalid Timestamp value: " + token.StringValue);
+                throw new InvalidProtocolBufferException($"Invalid Timestamp value: {token.StringValue}");
             }
             var dateTime = match.Groups["datetime"].Value;
             var subseconds = match.Groups["subseconds"].Value;