fix fuzzer test w/nested maps

update upb_MiniTable_SetSubMessage() to return false on failure
fix ClangTidy warnings in upb/message/test.cc

PiperOrigin-RevId: 505191802

upb/message/test.cc

@@ -25,6 +25,9 @@
  * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <string>
+#include <string_view>
+
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include "google/protobuf/test_messages_proto3.upb.h"
@@ -93,8 +96,8 @@ TEST(MessageTest, Extensions) {
   VerifyMessage(ext_msg2);
 
   // Test round-trip through JSON format.
-  size_t json_size =
-      upb_JsonEncode(ext_msg, m.ptr(), defpool.ptr(), 0, NULL, 0, status.ptr());
+  size_t json_size = upb_JsonEncode(ext_msg, m.ptr(), defpool.ptr(), 0, nullptr,
+                                    0, status.ptr());
   char* json_buf =
       static_cast<char*>(upb_Arena_Malloc(arena.ptr(), json_size + 1));
   upb_JsonEncode(ext_msg, m.ptr(), defpool.ptr(), 0, json_buf, json_size + 1,
@@ -153,8 +156,8 @@ TEST(MessageTest, MessageSet) {
   VerifyMessageSet(ext_msg2);
 
   // Test round-trip through JSON format.
-  size_t json_size =
-      upb_JsonEncode(ext_msg, m.ptr(), defpool.ptr(), 0, NULL, 0, status.ptr());
+  size_t json_size = upb_JsonEncode(ext_msg, m.ptr(), defpool.ptr(), 0, nullptr,
+                                    0, status.ptr());
   char* json_buf =
       static_cast<char*>(upb_Arena_Malloc(arena.ptr(), json_size + 1));
   upb_JsonEncode(ext_msg, m.ptr(), defpool.ptr(), 0, json_buf, json_size + 1,
@@ -309,13 +312,14 @@ TEST(MessageTest, DecodeRequiredFieldsTopLevelMessage) {
   upb_test_EmptyMessage* empty_msg;
 
   // Succeeds, because we did not request required field checks.
-  test_msg = upb_test_TestRequiredFields_parse(NULL, 0, arena.ptr());
+  test_msg = upb_test_TestRequiredFields_parse(nullptr, 0, arena.ptr());
   EXPECT_NE(nullptr, test_msg);
 
   // Fails, because required fields are missing.
-  EXPECT_EQ(kUpb_DecodeStatus_MissingRequired,
-            upb_Decode(NULL, 0, test_msg, &upb_test_TestRequiredFields_msg_init,
-                       NULL, kUpb_DecodeOption_CheckRequired, arena.ptr()));
+  EXPECT_EQ(
+      kUpb_DecodeStatus_MissingRequired,
+      upb_Decode(nullptr, 0, test_msg, &upb_test_TestRequiredFields_msg_init,
+                 nullptr, kUpb_DecodeOption_CheckRequired, arena.ptr()));
 
   upb_test_TestRequiredFields_set_required_int32(test_msg, 1);
   size_t size;
@@ -328,7 +332,7 @@ TEST(MessageTest, DecodeRequiredFieldsTopLevelMessage) {
   // payload is not empty.
   EXPECT_EQ(kUpb_DecodeStatus_MissingRequired,
             upb_Decode(serialized, size, test_msg,
-                       &upb_test_TestRequiredFields_msg_init, NULL,
+                       &upb_test_TestRequiredFields_msg_init, nullptr,
                        kUpb_DecodeOption_CheckRequired, arena.ptr()));
 
   empty_msg = upb_test_EmptyMessage_new(arena.ptr());
@@ -337,9 +341,10 @@ TEST(MessageTest, DecodeRequiredFieldsTopLevelMessage) {
   upb_test_TestRequiredFields_set_required_message(test_msg, empty_msg);
 
   // Succeeds, because required fields are present (though not in the input).
-  EXPECT_EQ(kUpb_DecodeStatus_Ok,
-            upb_Decode(NULL, 0, test_msg, &upb_test_TestRequiredFields_msg_init,
-                       NULL, kUpb_DecodeOption_CheckRequired, arena.ptr()));
+  EXPECT_EQ(
+      kUpb_DecodeStatus_Ok,
+      upb_Decode(nullptr, 0, test_msg, &upb_test_TestRequiredFields_msg_init,
+                 nullptr, kUpb_DecodeOption_CheckRequired, arena.ptr()));
 
   // Serialize a complete payload.
   serialized =
@@ -348,7 +353,7 @@ TEST(MessageTest, DecodeRequiredFieldsTopLevelMessage) {
   EXPECT_NE(0, size);
 
   upb_test_TestRequiredFields* test_msg2 = upb_test_TestRequiredFields_parse_ex(
-      serialized, size, NULL, kUpb_DecodeOption_CheckRequired, arena.ptr());
+      serialized, size, nullptr, kUpb_DecodeOption_CheckRequired, arena.ptr());
   EXPECT_NE(nullptr, test_msg2);
 
   // When we add an incomplete sub-message, this is not flagged by the parser.
@@ -357,7 +362,7 @@ TEST(MessageTest, DecodeRequiredFieldsTopLevelMessage) {
       test_msg2, upb_test_TestRequiredFields_new(arena.ptr()));
   EXPECT_EQ(kUpb_DecodeStatus_Ok,
             upb_Decode(serialized, size, test_msg2,
-                       &upb_test_TestRequiredFields_msg_init, NULL,
+                       &upb_test_TestRequiredFields_msg_init, nullptr,
                        kUpb_DecodeOption_CheckRequired, arena.ptr()));
 }
 
@@ -381,7 +386,7 @@ TEST(MessageTest, DecodeRequiredFieldsSubMessage) {
 
   // Parse error when verifying required fields, due to incomplete sub-message.
   EXPECT_EQ(nullptr, upb_test_SubMessageHasRequired_parse_ex(
-                         serialized, size, NULL,
+                         serialized, size, nullptr,
                          kUpb_DecodeOption_CheckRequired, arena.ptr()));
 
   upb_test_TestRequiredFields_set_required_int32(test_msg, 1);
@@ -394,7 +399,7 @@ TEST(MessageTest, DecodeRequiredFieldsSubMessage) {
 
   // No parse error; sub-message now is complete.
   EXPECT_NE(nullptr, upb_test_SubMessageHasRequired_parse_ex(
-                         serialized, size, NULL,
+                         serialized, size, nullptr,
                          kUpb_DecodeOption_CheckRequired, arena.ptr()));
 }
 
@@ -603,6 +608,12 @@ TEST(MessageTest, MapField) {
 //                                         -1960166338, 16809991);
 // }
 //
+// // This test encodes a map containing a msg wrapping another, empty msg.
+// TEST(FuzzTest, DecodeEncodeArbitrarySchemaAndPayloadRegressionMapMap) {
+//   DecodeEncodeArbitrarySchemaAndPayload(
+//       {{"%#G"}, {}, "", {}}, std::string("\022\002\022\000", 4), 0, 0);
+// }
+//
 // TEST(FuzzTest, GroupMap) {
 //   // Groups should not be allowed as maps, but we previously failed to prevent
 //   // this.

upb/mini_table/decode.c

@@ -979,14 +979,28 @@ bool upb_MiniTable_SetSubMessage(upb_MiniTable* table,
   UPB_ASSERT((uintptr_t)table->fields <= (uintptr_t)field &&
              (uintptr_t)field <
                  (uintptr_t)(table->fields + table->field_count));
-  // TODO: check these type invariants at runtime and return error to the
-  // caller if they are violated, instead of using an assert.
-  UPB_ASSERT(field->descriptortype == kUpb_FieldType_Message ||
-             field->descriptortype == kUpb_FieldType_Group);
-  if (sub->ext & kUpb_ExtMode_IsMapEntry) {
-    UPB_ASSERT(field->descriptortype == kUpb_FieldType_Message);
-    field->mode = (field->mode & ~kUpb_FieldMode_Mask) | kUpb_FieldMode_Map;
+  UPB_ASSERT(sub);
+
+  const bool sub_is_map = sub->ext & kUpb_ExtMode_IsMapEntry;
+
+  switch (field->descriptortype) {
+    case kUpb_FieldType_Message:
+      if (sub_is_map) {
+        const bool table_is_map = table->ext & kUpb_ExtMode_IsMapEntry;
+        if (UPB_UNLIKELY(table_is_map)) return false;
+
+        field->mode = (field->mode & ~kUpb_FieldMode_Mask) | kUpb_FieldMode_Map;
+      }
+      break;
+
+    case kUpb_FieldType_Group:
+      if (UPB_UNLIKELY(sub_is_map)) return false;
+      break;
+
+    default:
+      return false;
   }
+
   upb_MiniTableSub* table_sub = (void*)&table->subs[field->submsg_index];
   table_sub->submsg = sub;
   return true;
@@ -997,6 +1011,8 @@ bool upb_MiniTable_SetSubEnum(upb_MiniTable* table, upb_MiniTableField* field,
   UPB_ASSERT((uintptr_t)table->fields <= (uintptr_t)field &&
              (uintptr_t)field <
                  (uintptr_t)(table->fields + table->field_count));
+  UPB_ASSERT(sub);
+
   upb_MiniTableSub* table_sub = (void*)&table->subs[field->submsg_index];
   table_sub->subenum = sub;
   return true;

upb/mini_table/extension_internal.h

@@ -35,7 +35,9 @@
 #include "upb/port/def.inc"
 
 struct upb_MiniTableExtension {
+  // Do not move this field. We need to be able to alias pointers.
   upb_MiniTableField field;
+
   const upb_MiniTable* extendee;
   upb_MiniTableSub sub;  // NULL unless submessage or proto2 enum
 };

upb/test/fuzz_util.cc

@@ -64,20 +64,6 @@ class Builder {
     return input_->links[link_++];
   }
 
-  const upb_MiniTable* NextNonMapEntryMiniTable() {
-    if (mini_tables_.empty()) return nullptr;
-    size_t start = NextLink() % mini_tables_.size();
-    size_t next = start;
-    do {
-      const upb_MiniTable* mini_table = mini_tables_[next];
-      if ((mini_table->ext & kUpb_ExtMode_IsMapEntry) == 0) {
-        return mini_table;
-      }
-      next = NextLink() % mini_tables_.size();
-    } while (next != start);
-    return nullptr;
-  }
-
   const upb_MiniTable* NextMiniTable() {
     return mini_tables_.empty()
                ? nullptr
@@ -168,31 +154,20 @@ bool Builder::LinkMessages() {
       upb_MiniTableField* field =
           const_cast<upb_MiniTableField*>(&table->fields[i]);
       if (link_ == input_->links.size()) link_ = 0;
-      switch (field->descriptortype) {
-        case kUpb_FieldType_Message: {
-          const upb_MiniTable* sub = NextMiniTable();
-          // We should always have at least one message.
-          assert(sub);
-          if (!upb_MiniTable_SetSubMessage(table, field, sub)) return false;
-          break;
+      if (field->descriptortype == kUpb_FieldType_Message ||
+          field->descriptortype == kUpb_FieldType_Group) {
+        if (!upb_MiniTable_SetSubMessage(table, field, NextMiniTable())) {
+          return false;
         }
-        case kUpb_FieldType_Group: {
-          const upb_MiniTable* sub = NextNonMapEntryMiniTable();
-          // sub will be nullptr if no non-map entry messages are available.
-          if (sub) {
-            if (!upb_MiniTable_SetSubMessage(table, field, sub)) return false;
-          }
-          break;
-        }
-        case kUpb_FieldType_Enum: {
-          auto* et = NextEnumTable();
-          if (et) {
-            if (!upb_MiniTable_SetSubEnum(table, field, et)) return false;
-          } else {
-            // We don't have any sub-enums.  Override the field type so that it
-            // is not needed.
-            field->descriptortype = kUpb_FieldType_Int32;
-          }
+      }
+      if (field->descriptortype == kUpb_FieldType_Enum) {
+        auto* et = NextEnumTable();
+        if (et) {
+          if (!upb_MiniTable_SetSubEnum(table, field, et)) return false;
+        } else {
+          // We don't have any sub-enums.  Override the field type so that it is
+          // not needed.
+          field->descriptortype = kUpb_FieldType_Int32;
         }
       }
     }