Fixed a parser bug where closed enums are parsed incorrectly for non-repeated extensions.

The bug is triggered whenever we parse a value for a closed enum that is not a declared member of the enum.  The proper behavior in this case is to put the value to the unknown fields, leaving the extension unset.  The first part is being performed correctly -- we are indeed putting the value into the unknown fields.  But we currently detect this condition rather late in the decoding pipeline, after the extension has already been created.  This causes a stray `0` value to be left in the extension for this field (or, in the unlikely case that the message already contained a value for this field, the extension will contain whatever data was already present in the message).

The observable effects of this bug are:

1. If an invalid value is parsed for a closed enum extension, `HasExtension(foo)` will return true and `GetExtension(foo)` will return a 0, instead of the correct behavior, which is that `HasExtension(foo)` should return false.

2. If the message is later serialized, the extension will be serialized twice, once from the extension list (which will serialize a 0 value) and once from the unknown fields (which will serialize the correct value).  This should not be observable in most cases, since the bug only occurs for non-repeated fields, and parsers will overwrite the existing value if a non-repeated field is encountered multiple times on the wire.

PiperOrigin-RevId: 799202921

Author: haberman

python/google/protobuf/internal/message_test.py

@@ -1492,25 +1492,15 @@ def testClosedEnumExtension(self, message_module):
     m.ParseFromString(b'\xa8\x01\x7f')
     unknown = unknown_fields.UnknownFieldSet(m)
 
-    # All implementations put the data into unknown fields.
+    # The data is present in unknown fields.
     self.assertEqual(unknown[0].field_number, 21)
     self.assertEqual(unknown[0].wire_type, wire_format.WIRETYPE_VARINT)
     self.assertEqual(unknown[0].data, 0x7f)
 
-    if api_implementation.Type() == 'upb':
-      # upb incorrectly leaves a '0' value in the actual extension field.
-      # TODO: fix upb to not do this.
-      self.assertTrue(
-          m.HasExtension(unittest_pb2.optional_nested_enum_extension)
-      )
-      self.assertEqual(
-          m.Extensions[unittest_pb2.optional_nested_enum_extension], 0
-      )
-    else:
-      # Pure python and cpp extensions correctly have no extension present.
-      self.assertFalse(
-          m.HasExtension(unittest_pb2.optional_nested_enum_extension)
-      )
+    # There is no extension present.
+    self.assertFalse(
+        m.HasExtension(unittest_pb2.optional_nested_enum_extension)
+    )
 
   def testAssignBoolToInt(self, message_module):
     with warnings.catch_warnings(record=True) as w:

upb/wire/decode.c

@@ -75,7 +75,6 @@ enum {
   kUpb_DecodeOp_Scalar1Byte = 0,
   kUpb_DecodeOp_Scalar4Byte = 2,
   kUpb_DecodeOp_Scalar8Byte = 3,
-  kUpb_DecodeOp_Enum = 1,
 
   // Scalar/repeated ops.
   kUpb_DecodeOp_String = 4,
@@ -221,8 +220,8 @@ static void _upb_Decoder_MungeInt32(wireval* val) {
   }
 }
 
-static void _upb_Decoder_Munge(int type, wireval* val) {
-  switch (type) {
+static void _upb_Decoder_Munge(const upb_MiniTableField* field, wireval* val) {
+  switch (field->UPB_PRIVATE(descriptortype)) {
     case kUpb_FieldType_Bool:
       val->bool_val = val->uint64_val != 0;
       break;
@@ -238,9 +237,10 @@ static void _upb_Decoder_Munge(int type, wireval* val) {
     }
     case kUpb_FieldType_Int32:
     case kUpb_FieldType_UInt32:
-    case kUpb_FieldType_Enum:
       _upb_Decoder_MungeInt32(val);
       break;
+    case kUpb_FieldType_Enum:
+      UPB_UNREACHABLE();
   }
 }
 
@@ -385,13 +385,9 @@ static char* upb_Decoder_EncodeVarint32(uint32_t val, char* ptr) {
 }
 
 UPB_FORCEINLINE
-bool _upb_Decoder_CheckEnum(upb_Decoder* d, const char* ptr, upb_Message* msg,
-                            const upb_MiniTableEnum* e,
-                            const upb_MiniTableField* field, wireval* val) {
-  const uint32_t v = val->uint32_val;
-
-  if (UPB_LIKELY(upb_MiniTableEnum_CheckValue(e, v))) return true;
-
+void _upb_Decoder_AddEnumValueToUnknown(upb_Decoder* d, upb_Message* msg,
+                                        const upb_MiniTableField* field,
+                                        wireval* val) {
   // Unrecognized enum goes into unknown fields.
   // For packed fields the tag could be arbitrarily far in the past,
   // so we just re-encode the tag and value here.
@@ -403,27 +399,12 @@ bool _upb_Decoder_CheckEnum(upb_Decoder* d, const char* ptr, upb_Message* msg,
   char buf[2 * kUpb_Decoder_EncodeVarint32MaxSize];
   char* end = buf;
   end = upb_Decoder_EncodeVarint32(tag, end);
-  end = upb_Decoder_EncodeVarint32(v, end);
+  end = upb_Decoder_EncodeVarint32(val->uint64_val, end);
 
   if (!UPB_PRIVATE(_upb_Message_AddUnknown)(unknown_msg, buf, end - buf,
                                             &d->arena, NULL)) {
     _upb_Decoder_ErrorJmp(d, kUpb_DecodeStatus_OutOfMemory);
   }
-  return false;
-}
-
-UPB_NOINLINE
-static const char* _upb_Decoder_DecodeEnumArray(
-    upb_Decoder* d, const char* ptr, upb_Message* msg, upb_Array* arr,
-    const upb_MiniTableSubInternal* subs, const upb_MiniTableField* field,
-    wireval* val) {
-  const upb_MiniTableEnum* e = _upb_MiniTableSubs_EnumByField(subs, field);
-  if (!_upb_Decoder_CheckEnum(d, ptr, msg, e, field, val)) return ptr;
-  void* mem = UPB_PTR_AT(upb_Array_MutableDataPtr(arr),
-                         arr->UPB_PRIVATE(size) * 4, void);
-  arr->UPB_PRIVATE(size)++;
-  memcpy(mem, val, 4);
-  return ptr;
 }
 
 UPB_FORCEINLINE
@@ -476,7 +457,7 @@ const char* _upb_Decoder_DecodeVarintPacked(upb_Decoder* d, const char* ptr,
   while (!_upb_Decoder_IsDone(d, &ptr)) {
     wireval elem;
     ptr = _upb_Decoder_DecodeVarint(d, ptr, &elem.uint64_val);
-    _upb_Decoder_Munge(field->UPB_PRIVATE(descriptortype), &elem);
+    _upb_Decoder_Munge(field, &elem);
     if (_upb_Decoder_Reserve(d, arr, 1)) {
       out = UPB_PTR_AT(upb_Array_MutableDataPtr(arr),
                        arr->UPB_PRIVATE(size) << lg2, void);
@@ -501,8 +482,8 @@ static const char* _upb_Decoder_DecodeEnumPacked(
   while (!_upb_Decoder_IsDone(d, &ptr)) {
     wireval elem;
     ptr = _upb_Decoder_DecodeVarint(d, ptr, &elem.uint64_val);
-    _upb_Decoder_MungeInt32(&elem);
-    if (!_upb_Decoder_CheckEnum(d, ptr, msg, e, field, &elem)) {
+    if (!upb_MiniTableEnum_CheckValue(e, elem.uint64_val)) {
+      _upb_Decoder_AddEnumValueToUnknown(d, msg, field, &elem);
       continue;
     }
     if (_upb_Decoder_Reserve(d, arr, 1)) {
@@ -585,8 +566,6 @@ static const char* _upb_Decoder_DecodeToArray(
     case OP_VARPCK_LG2(3):
       return _upb_Decoder_DecodeVarintPacked(d, ptr, arr, val, field,
                                              op - OP_VARPCK_LG2(0));
-    case kUpb_DecodeOp_Enum:
-      return _upb_Decoder_DecodeEnumArray(d, ptr, msg, arr, subs, field, val);
     case kUpb_DecodeOp_PackedEnum:
       return _upb_Decoder_DecodeEnumPacked(d, ptr, msg, arr, subs, field, val);
     default:
@@ -710,13 +689,6 @@ static const char* _upb_Decoder_DecodeToSubMessage(
   void* mem = UPB_PTR_AT(msg, field->UPB_PRIVATE(offset), void);
   int type = field->UPB_PRIVATE(descriptortype);
 
-  if (UPB_UNLIKELY(op == kUpb_DecodeOp_Enum) &&
-      !_upb_Decoder_CheckEnum(d, ptr, msg,
-                              _upb_MiniTableSubs_EnumByField(subs, field),
-                              field, val)) {
-    return ptr;
-  }
-
   // Set presence if necessary.
   if (UPB_PRIVATE(_upb_MiniTableField_HasHasbit)(field)) {
     UPB_PRIVATE(_upb_Message_SetHasbit)(msg, field);
@@ -756,7 +728,6 @@ static const char* _upb_Decoder_DecodeToSubMessage(
     case kUpb_DecodeOp_Scalar8Byte:
       memcpy(mem, val, 8);
       break;
-    case kUpb_DecodeOp_Enum:
     case kUpb_DecodeOp_Scalar4Byte:
       memcpy(mem, val, 4);
       break;
@@ -970,7 +941,7 @@ static int _upb_Decoder_GetVarintOp(const upb_MiniTableField* field) {
       [kUpb_FieldType_Message] = kUpb_DecodeOp_UnknownField,
       [kUpb_FieldType_Bytes] = kUpb_DecodeOp_UnknownField,
       [kUpb_FieldType_UInt32] = kUpb_DecodeOp_Scalar4Byte,
-      [kUpb_FieldType_Enum] = kUpb_DecodeOp_Enum,
+      [kUpb_FieldType_Enum] = kUpb_DecodeOp_Scalar4Byte,
       [kUpb_FieldType_SFixed32] = kUpb_DecodeOp_UnknownField,
       [kUpb_FieldType_SFixed64] = kUpb_DecodeOp_UnknownField,
       [kUpb_FieldType_SInt32] = kUpb_DecodeOp_Scalar4Byte,
@@ -1096,8 +1067,20 @@ const char* _upb_Decoder_DecodeWireValue(upb_Decoder* d, const char* ptr,
   switch (wire_type) {
     case kUpb_WireType_Varint:
       ptr = _upb_Decoder_DecodeVarint(d, ptr, &val->uint64_val);
+      if (upb_MiniTableField_IsClosedEnum(field)) {
+        const upb_MiniTableEnum* e =
+            upb_MiniTableField_IsExtension(field)
+                ? upb_MiniTableExtension_GetSubEnum(
+                      (const upb_MiniTableExtension*)field)
+                : upb_MiniTable_GetSubEnumTable(mt, field);
+        if (!upb_MiniTableEnum_CheckValue(e, val->uint64_val)) {
+          *op = kUpb_DecodeOp_UnknownField;
+          return ptr;
+        }
+      } else {
+        _upb_Decoder_Munge(field, val);
+      }
       *op = _upb_Decoder_GetVarintOp(field);
-      _upb_Decoder_Munge(field->UPB_PRIVATE(descriptortype), val);
       return ptr;
     case kUpb_WireType_32Bit:
       *op = kUpb_DecodeOp_Scalar4Byte;