Fixed non-conformance in JSON parsing for empty strings in numeric fields.

Ruby & PHP will raise a warning, but will also raise a ParseError in the next minor release.

PiperOrigin-RevId: 696138522

php/ext/google/protobuf/message.c

@@ -722,12 +722,20 @@ PHP_METHOD(Message, mergeFromJsonString) {
   }
 
   upb_Status_Clear(&status);
-  if (!upb_JsonDecode(data, data_len, intern->msg, intern->desc->msgdef,
-                      DescriptorPool_GetSymbolTable(), options, arena,
-                      &status)) {
-    zend_throw_exception_ex(NULL, 0, "Error occurred during parsing: %s",
-                            upb_Status_ErrorMessage(&status));
-    return;
+  int result = upb_JsonDecodeDetectingNonconformance(
+      data, data_len, intern->msg, intern->desc->msgdef,
+      DescriptorPool_GetSymbolTable(), options, arena, &status);
+
+  switch (result) {
+    case kUpb_JsonDecodeResult_Ok:
+      break;
+    case kUpb_JsonDecodeResult_OkWithEmptyStringNumerics:
+      zend_error(E_USER_WARNING, "%s", upb_Status_ErrorMessage(&status));
+      return;
+    case kUpb_JsonDecodeResult_Error:
+      zend_throw_exception_ex(NULL, 0, "Error occurred during parsing: %s",
+                              upb_Status_ErrorMessage(&status));
+      return;
   }
 }
 

ruby/ext/google/protobuf_c/message.c

@@ -1038,11 +1038,21 @@ static VALUE Message_decode_json(int argc, VALUE* argv, VALUE klass) {
 
   upb_Status_Clear(&status);
   const upb_DefPool* pool = upb_FileDef_Pool(upb_MessageDef_File(msg->msgdef));
-  if (!upb_JsonDecode(RSTRING_PTR(data), RSTRING_LEN(data),
-                      (upb_Message*)msg->msg, msg->msgdef, pool, options,
-                      Arena_get(msg->arena), &status)) {
-    rb_raise(cParseError, "Error occurred during parsing: %s",
-             upb_Status_ErrorMessage(&status));
+
+  int result = upb_JsonDecodeDetectingNonconformance(
+      RSTRING_PTR(data), RSTRING_LEN(data), (upb_Message*)msg->msg,
+      msg->msgdef, pool, options, Arena_get(msg->arena), &status);
+
+  switch (result) {
+    case kUpb_JsonDecodeResult_Ok:
+      break;
+    case kUpb_JsonDecodeResult_OkWithEmptyStringNumerics:
+      rb_warn("%s", upb_Status_ErrorMessage(&status));
+      break;
+    case kUpb_JsonDecodeResult_Error:
+      rb_raise(cParseError, "Error occurred during parsing: %s",
+               upb_Status_ErrorMessage(&status));
+      break;
   }
 
   return msg_rb;

ruby/lib/google/protobuf/ffi/ffi.rb

@@ -36,6 +36,11 @@ module Google
       ## JSON Decoding options
       Upb_JsonDecode_IgnoreUnknown = 1
 
+      ## JSON Decoding results
+      Upb_JsonDecodeResult_Ok = 0
+      Upb_JsonDecodeResult_OkWithEmptyStringNumerics = 1
+      Upb_JsonDecodeResult_Error = 2
+
       typedef :pointer, :Array
       typedef :pointer, :DefPool
       typedef :pointer, :EnumValueDef

ruby/lib/google/protobuf/ffi/message.rb

@@ -17,7 +17,7 @@ module Google
       attach_function :get_message_has,         :upb_Message_HasFieldByDef,   [:Message, FieldDescriptor], :bool
       attach_function :set_message_field,       :upb_Message_SetFieldByDef,   [:Message, FieldDescriptor, MessageValue.by_value, Internal::Arena], :bool
       attach_function :encode_message,          :upb_Encode,                  [:Message, MiniTable.by_ref, :size_t, Internal::Arena, :pointer, :pointer], EncodeStatus
-      attach_function :json_decode_message,     :upb_JsonDecode,              [:binary_string, :size_t, :Message, Descriptor, :DefPool, :int, Internal::Arena, Status.by_ref], :bool
+      attach_function :json_decode_message_detecting_nonconformance,     :upb_JsonDecodeDetectingNonconformance,   [:binary_string, :size_t, :Message, Descriptor, :DefPool, :int, Internal::Arena, Status.by_ref], :int
       attach_function :json_encode_message,     :upb_JsonEncode,              [:Message, Descriptor, :DefPool, :int, :binary_string, :size_t, Status.by_ref], :size_t
       attach_function :decode_message,          :upb_Decode,                  [:binary_string, :size_t, :Message, MiniTable.by_ref, :ExtensionRegistry, :int, Internal::Arena], DecodeStatus
       attach_function :get_mutable_message,     :upb_Message_Mutable,         [:Message, FieldDescriptor, Internal::Arena], MutableMessageValue.by_value
@@ -270,7 +270,11 @@ module Google
             message = new
             pool_def = message.class.descriptor.instance_variable_get(:@descriptor_pool).descriptor_pool
             status = Google::Protobuf::FFI::Status.new
-            unless Google::Protobuf::FFI.json_decode_message(data, data.bytesize, message.instance_variable_get(:@msg), message.class.descriptor, pool_def, decoding_options, message.instance_variable_get(:@arena), status)
+            result = Google::Protobuf::FFI.json_decode_message_detecting_nonconformance(data, data.bytesize, message.instance_variable_get(:@msg), message.class.descriptor, pool_def, decoding_options, message.instance_variable_get(:@arena), status)
+            case result
+            when Google::Protobuf::FFI::Upb_JsonDecodeResult_OkWithEmptyStringNumerics
+              warn Google::Protobuf::FFI.error_message(status)
+            when Google::Protobuf::FFI::Upb_JsonDecodeResult_Error
               raise ParseError.new "Error occurred during parsing: #{Google::Protobuf::FFI.error_message(status)}"
             end
             message

ruby/tests/encode_decode_test.rb

@@ -3,14 +3,75 @@
 # generated_code.rb is in the same directory as this test.
 $LOAD_PATH.unshift(File.expand_path(File.dirname(__FILE__)))
 
+require 'basic_test_proto2_pb'
 require 'generated_code_pb'
 require 'google/protobuf/well_known_types'
 require 'test/unit'
 
+module CaptureWarnings
+  @@warnings = nil
+
+  module_function
+
+  def warn(message, category: nil, **kwargs)
+    if @@warnings
+      @@warnings << message
+    else
+      super
+    end
+  end
+
+  def capture
+    @@warnings = []
+    yield
+    @@warnings
+  ensure
+    @@warnings = nil
+  end
+end
+
+Warning.extend CaptureWarnings
+
 def hex2bin(s)
   s.scan(/../).map { |x| x.hex.chr }.join
 end
 
+class NonConformantNumericsTest < Test::Unit::TestCase
+  def test_empty_json_numerics
+    if defined? JRUBY_VERSION and Google::Protobuf::IMPLEMENTATION != :FFI
+      # In a future version, CRuby and JRuby FFI will also have this behavior.
+      assert_raises Google::Protobuf::ParseError do
+        msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalInt32":""}')
+      end
+    else
+      warnings = CaptureWarnings.capture {
+        msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalInt32":""}')
+        assert_equal 0, msg.optional_int32
+        assert msg.has_optional_int32?
+      }
+      assert_equal 1, warnings.size
+      assert_match "Empty string is not a valid number (field: basic_test_proto2.TestMessage.optional_int32)", warnings[0]
+    end
+  end
+
+  def test_trailing_non_numeric_characters
+    if defined? JRUBY_VERSION and Google::Protobuf::IMPLEMENTATION != :FFI
+      # In a future version, CRuby and JRuby FFI will also have this behavior.
+      assert_raises Google::Protobuf::ParseError do
+        msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalDouble":"123abc"}')
+      end
+    else
+      warnings = CaptureWarnings.capture {
+        msg = ::BasicTestProto2::TestMessage.decode_json('{"optionalDouble":"123abc"}')
+        assert_equal 123, msg.optional_double
+        assert msg.has_optional_double?
+      }
+      assert_equal 1, warnings.size
+      assert_match "Non-number characters in quoted number (field: basic_test_proto2.TestMessage.optional_double)", warnings[0]
+    end
+  end
+end
+
 class EncodeDecodeTest < Test::Unit::TestCase
   def test_discard_unknown
     # Test discard unknown in message.

upb/conformance/conformance_upb_failures.txt

@@ -1,11 +1 @@
-Required.*.JsonInput.DoubleFieldEmptyString                 # Should have failed to parse, but didn't.
-Required.*.JsonInput.FloatFieldEmptyString                  # Should have failed to parse, but didn't.
-Required.*.JsonInput.Int32FieldEmptyString                  # Should have failed to parse, but didn't.
-Required.*.JsonInput.Int64FieldEmptyString                  # Should have failed to parse, but didn't.
-Required.*.JsonInput.Uint32FieldEmptyString                 # Should have failed to parse, but didn't.
-Required.*.JsonInput.Uint64FieldEmptyString                 # Should have failed to parse, but didn't.
-Required.*.JsonInput.DoubleFieldStringValueNonNumeric       # Should have failed to parse, but didn't.
-Required.*.JsonInput.DoubleFieldStringValuePartiallyNumeric # Should have failed to parse, but didn't.
-Required.*.JsonInput.FloatFieldStringValueNonNumeric        # Should have failed to parse, but didn't.
-Required.*.JsonInput.FloatFieldStringValuePartiallyNumeric  # Should have failed to parse, but didn't.
 Required.*.JsonInput.Int32FieldQuotedExponentialValue.*     # Failed to parse input or produce output.

upb/json/decode.c

@@ -41,6 +41,7 @@ typedef struct {
   upb_Arena* arena; /* TODO: should we have a tmp arena for tmp data? */
   const upb_DefPool* symtab;
   int depth;
+  int result;
   upb_Status* status;
   jmp_buf err;
   int line;
@@ -670,6 +671,16 @@ static int64_t jsondec_strtoint64(jsondec* d, upb_StringView str) {
   return ret;
 }
 
+static void jsondec_checkempty(jsondec* d, upb_StringView str,
+                               const upb_FieldDef* f) {
+  if (str.size != 0) return;
+  d->result = kUpb_JsonDecodeResult_OkWithEmptyStringNumerics;
+  upb_Status_SetErrorFormat(d->status,
+                            "Empty string is not a valid number (field: %s). "
+                            "This will be an error in a future version.",
+                            upb_FieldDef_FullName(f));
+}
+
 /* Primitive value types ******************************************************/
 
 /* Parse INT32 or INT64 value. */
@@ -691,6 +702,7 @@ static upb_MessageValue jsondec_int(jsondec* d, const upb_FieldDef* f) {
     }
     case JD_STRING: {
       upb_StringView str = jsondec_string(d);
+      jsondec_checkempty(d, str, f);
       val.int64_val = jsondec_strtoint64(d, str);
       break;
     }
@@ -728,6 +740,7 @@ static upb_MessageValue jsondec_uint(jsondec* d, const upb_FieldDef* f) {
     }
     case JD_STRING: {
       upb_StringView str = jsondec_string(d);
+      jsondec_checkempty(d, str, f);
       val.uint64_val = jsondec_strtouint64(d, str);
       break;
     }
@@ -756,14 +769,26 @@ static upb_MessageValue jsondec_double(jsondec* d, const upb_FieldDef* f) {
       break;
     case JD_STRING:
       str = jsondec_string(d);
-      if (jsondec_streql(str, "NaN")) {
+      if (str.size == 0) {
+        jsondec_checkempty(d, str, f);
+        val.double_val = 0.0;
+      } else if (jsondec_streql(str, "NaN")) {
         val.double_val = NAN;
       } else if (jsondec_streql(str, "Infinity")) {
         val.double_val = INFINITY;
       } else if (jsondec_streql(str, "-Infinity")) {
         val.double_val = -INFINITY;
       } else {
-        val.double_val = strtod(str.data, NULL);
+        char* end;
+        val.double_val = strtod(str.data, &end);
+        if (end != str.data + str.size) {
+          d->result = kUpb_JsonDecodeResult_OkWithEmptyStringNumerics;
+          upb_Status_SetErrorFormat(
+              d->status,
+              "Non-number characters in quoted number (field: %s). "
+              "This will be an error in a future version.",
+              upb_FieldDef_FullName(f));
+        }
       }
       break;
     default:
@@ -1505,10 +1530,10 @@ static void jsondec_wellknown(jsondec* d, upb_Message* msg,
   }
 }
 
-static bool upb_JsonDecoder_Decode(jsondec* const d, upb_Message* const msg,
-                                   const upb_MessageDef* const m) {
+static int upb_JsonDecoder_Decode(jsondec* const d, upb_Message* const msg,
+                                  const upb_MessageDef* const m) {
   UPB_ASSERT(!upb_Message_IsFrozen(msg));
-  if (UPB_SETJMP(d->err)) return false;
+  if (UPB_SETJMP(d->err)) return kUpb_JsonDecodeResult_Error;
 
   jsondec_tomsg(d, msg, m);
 
@@ -1517,16 +1542,19 @@ static bool upb_JsonDecoder_Decode(jsondec* const d, upb_Message* const msg,
   jsondec_consumews(d);
 
   if (d->ptr == d->end) {
-    return true;
+    return d->result;
   } else {
     jsondec_seterrmsg(d, "unexpected trailing characters");
-    return false;
+    return kUpb_JsonDecodeResult_Error;
   }
 }
 
-bool upb_JsonDecode(const char* buf, size_t size, upb_Message* msg,
-                    const upb_MessageDef* m, const upb_DefPool* symtab,
-                    int options, upb_Arena* arena, upb_Status* status) {
+int upb_JsonDecodeDetectingNonconformance(const char* buf, size_t size,
+                                          upb_Message* msg,
+                                          const upb_MessageDef* m,
+                                          const upb_DefPool* symtab,
+                                          int options, upb_Arena* arena,
+                                          upb_Status* status) {
   UPB_ASSERT(!upb_Message_IsFrozen(msg));
   jsondec d;
 
@@ -1539,6 +1567,7 @@ bool upb_JsonDecode(const char* buf, size_t size, upb_Message* msg,
   d.status = status;
   d.options = options;
   d.depth = 64;
+  d.result = kUpb_JsonDecodeResult_Ok;
   d.line = 1;
   d.line_begin = d.ptr;
   d.debug_field = NULL;

upb/json/decode.h

@@ -8,6 +8,11 @@
 #ifndef UPB_JSON_DECODE_H_
 #define UPB_JSON_DECODE_H_
 
+#include <stddef.h>
+
+#include "upb/base/status.h"
+#include "upb/mem/arena.h"
+#include "upb/message/message.h"
 #include "upb/reflection/def.h"
 
 // Must be last.
@@ -19,9 +24,27 @@ extern "C" {
 
 enum { upb_JsonDecode_IgnoreUnknown = 1 };
 
-UPB_API bool upb_JsonDecode(const char* buf, size_t size, upb_Message* msg,
-                            const upb_MessageDef* m, const upb_DefPool* symtab,
-                            int options, upb_Arena* arena, upb_Status* status);
+enum {
+  kUpb_JsonDecodeResult_Ok = 0,
+  kUpb_JsonDecodeResult_OkWithEmptyStringNumerics = 1,
+  kUpb_JsonDecodeResult_Error = 2,
+};
+
+UPB_API int upb_JsonDecodeDetectingNonconformance(const char* buf, size_t size,
+                                                  upb_Message* msg,
+                                                  const upb_MessageDef* m,
+                                                  const upb_DefPool* symtab,
+                                                  int options, upb_Arena* arena,
+                                                  upb_Status* status);
+
+UPB_API_INLINE bool upb_JsonDecode(const char* buf, size_t size,
+                                   upb_Message* msg, const upb_MessageDef* m,
+                                   const upb_DefPool* symtab, int options,
+                                   upb_Arena* arena, upb_Status* status) {
+  return upb_JsonDecodeDetectingNonconformance(buf, size, msg, m, symtab,
+                                               options, arena, status) ==
+         kUpb_JsonDecodeResult_Ok;
+}
 
 #ifdef __cplusplus
 } /* extern "C" */