From 5a7644b2d069d8e834cd41c677ed70e89e66920e Mon Sep 17 00:00:00 2001
From: Joshua Haberman <haberman@google.com>
Date: Tue, 6 Sep 2022 10:27:55 -0700
Subject: [PATCH] Fixed fuzz bug in upb.
Extending a MessageSet with a non-message extension was causing crashes that would manifest in various ways.
PiperOrigin-RevId: 472496259
---
upb/fuzz_test_util.cc | 8 ++++----
upb/mini_table.c | 23 ++++++++++++++++++++---
upb/mini_table.h | 1 +
upb/msg_test.cc | 36 ++++++++++++++++++++++++++++++++++++
upbc/file_layout.cc | 6 +++++-
5 files changed, 66 insertions(+), 8 deletions(-)
diff --git a/upb/fuzz_test_util.cc b/upb/fuzz_test_util.cc
index 12de4a9283..219e4a3294 100644
--- a/upb/fuzz_test_util.cc
+++ b/upb/fuzz_test_util.cc
@@ -111,8 +111,6 @@ void Builder::BuildEnums() {
bool Builder::LinkExtension(upb_MiniTable_Extension* ext) {
upb_MiniTable_Field* field = &ext->field;
- ext->extendee = NextMiniTable();
- if (!ext->extendee) return false;
if (field->descriptortype == kUpb_FieldType_Message ||
field->descriptortype == kUpb_FieldType_Group) {
auto mt = NextMiniTable();
@@ -140,8 +138,10 @@ void Builder::BuildExtensions(upb_ExtensionRegistry** exts) {
upb_MiniTable_Extension* ext = reinterpret_cast<upb_MiniTable_Extension*>(
upb_Arena_Malloc(arena_, sizeof(*ext)));
upb_MiniTable_Sub sub;
- ptr =
- upb_MiniTable_BuildExtension(ptr, end - ptr, ext, sub, status.ptr());
+ const upb_MiniTable* extendee = NextMiniTable();
+ if (!extendee) break;
+ ptr = upb_MiniTable_BuildExtension(ptr, end - ptr, ext, extendee, sub,
+ status.ptr());
if (!ptr) break;
if (!LinkExtension(ext)) continue;
if (_upb_extreg_get(*exts, ext->extendee, ext->field.number)) continue;
diff --git a/upb/mini_table.c b/upb/mini_table.c
index 0b59607053..3d13780a04 100644
--- a/upb/mini_table.c
+++ b/upb/mini_table.c
@@ -1102,6 +1102,7 @@ upb_MiniTable_Enum* upb_MiniTable_BuildEnum(const char* data, size_t len,
const char* upb_MiniTable_BuildExtension(const char* data, size_t len,
upb_MiniTable_Extension* ext,
+ const upb_MiniTable* extendee,
upb_MiniTable_Sub sub,
upb_Status* status) {
upb_MtDecoder decoder = {
@@ -1117,9 +1118,25 @@ const char* upb_MiniTable_BuildExtension(const char* data, size_t len,
uint16_t count = 0;
const char* ret =
upb_MtDecoder_Parse(&decoder, data, len, ext, sizeof(*ext), &count, NULL);
- ext->field.mode |= kUpb_LabelFlags_IsExtension;
- ext->field.offset = 0;
- ext->field.presence = 0;
+ if (!ret) return NULL;
+
+ upb_MiniTable_Field* f = &ext->field;
+
+ f->mode |= kUpb_LabelFlags_IsExtension;
+ f->offset = 0;
+ f->presence = 0;
+
+ if (extendee->ext & kUpb_ExtMode_IsMessageSet) {
+ // Extensions of MessageSet must be messages.
+ if (!upb_IsSubMessage(f)) return NULL;
+
+ // Extensions of MessageSet must be non-repeating.
+ if ((f->mode & kUpb_FieldMode_Mask) == kUpb_FieldMode_Array) return NULL;
+ }
+
+ ext->extendee = extendee;
+ ext->sub = sub;
+
return ret;
}
diff --git a/upb/mini_table.h b/upb/mini_table.h
index 238c6d7222..d71ebcf320 100644
--- a/upb/mini_table.h
+++ b/upb/mini_table.h
@@ -156,6 +156,7 @@ void upb_MiniTable_SetSubEnum(upb_MiniTable* table, upb_MiniTable_Field* field,
const char* upb_MiniTable_BuildExtension(const char* data, size_t len,
upb_MiniTable_Extension* ext,
+ const upb_MiniTable* extendee,
upb_MiniTable_Sub sub,
upb_Status* status);
diff --git a/upb/msg_test.cc b/upb/msg_test.cc
index 68fd6267ee..9e1548ae9d 100644
--- a/upb/msg_test.cc
+++ b/upb/msg_test.cc
@@ -534,4 +534,40 @@ TEST(MessageTest, MapField) {
// "\010\002", 342248070, -806315555);
// }
//
+// TEST(FuzzTest, DecodeExtendMessageSetWithNonMessage) {
+// DecodeEncodeArbitrarySchemaAndPayload(
+// {{"\n"}, {""}, ".\244", {}}, "\013\032\005\212a#\365\336\020\001\226",
+// 14803219, 670718349);
+// }
+//
+// TEST(FuzzTest, DecodeExtendMessageSetWithNonMessage2) {
+// DecodeEncodeArbitrarySchemaAndPayload({{"\n", "G", "\n", "\274", ""},
+// {"", "\030"},
+// "_@",
+// {4294967295, 2147483647}},
+// std::string("\013\032\000\220", 4),
+// 279975758, 1647495141);
+// }
+//
+// TEST(FuzzTest, DecodeExtendMessageSetWithNonMessage3) {
+// DecodeEncodeArbitrarySchemaAndPayload(
+// {{"\n"}, {"B", ""}, "\212:b", {11141121}},
+// "\013\032\004\357;7\363\020\001\346\240\200\201\271", 399842149,
+// -452966025);
+// }
+//
+// TEST(FuzzTest, DecodeExtendMessageSetWithNonMessage4) {
+// DecodeEncodeArbitrarySchemaAndPayload(
+// {{"\n", "3\340", "\354"}, {}, "B}G", {4294967295, 4082331310}},
+// "\013\032\004\244B\331\255\020\001\220\224\243\350\t", -561523015,
+// 1683327312);
+// }
+//
+// TEST(FuzzTest, DecodeExtendMessageSetWithNonMessage5) {
+// DecodeEncodeArbitrarySchemaAndPayload(
+// {{"\n"}, {""}, "kB", {0}},
+// "x\203\251\006\013\032\002S\376\010\273\'\020\014\365\207\244\234",
+// -696925610, -654590577);
+// }
+//
// end:google_only
diff --git a/upbc/file_layout.cc b/upbc/file_layout.cc
index 49268aab27..f9dbac39f0 100644
--- a/upbc/file_layout.cc
+++ b/upbc/file_layout.cc
@@ -265,8 +265,12 @@ void FilePlatformLayout::BuildExtensions(const protobuf::FileDescriptor* fd) {
GetFieldModifiers(f));
upb_MiniTable_Extension& ext = extension_map_[f];
upb_MiniTable_Sub sub;
+ // The extendee may be from another file, so we build a temporary MiniTable
+ // for it, just for the purpose of building the extension.
+ // Note, we are not caching so this could use more memory than is necessary.
+ upb_MiniTable* extendee = MakeMiniTable(f->containing_type());
bool ok = upb_MiniTable_BuildExtension(e.data().data(), e.data().size(),
- &ext, sub, status.ptr());
+ &ext, extendee, sub, status.ptr());
if (!ok) {
// TODO(haberman): Use ABSL CHECK() when it is available.
fprintf(stderr, "Error building mini-table: %s\n",