diff --git a/upb/message/test.cc b/upb/message/test.cc index fbd6210885..717d8a8777 100644 --- a/upb/message/test.cc +++ b/upb/message/test.cc @@ -25,6 +25,9 @@ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ +#include +#include + #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(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(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. diff --git a/upb/mini_table/decode.c b/upb/mini_table/decode.c index 405e7b70b4..fea9643a74 100644 --- a/upb/mini_table/decode.c +++ b/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; diff --git a/upb/mini_table/extension_internal.h b/upb/mini_table/extension_internal.h index 73499c3f93..2a29ff505e 100644 --- a/upb/mini_table/extension_internal.h +++ b/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 }; diff --git a/upb/test/fuzz_util.cc b/upb/test/fuzz_util.cc index 0009cf6271..8fafc000a3 100644 --- a/upb/test/fuzz_util.cc +++ b/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(&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; } } }