Fix upb's json decoder ignoring trailing characters after a successfully parsed object.

protobuf-github-bot · copybara-github · commit fa15c2160eae · 2023-11-08T05:04:58.000-08:00
This is a breaking change since the JSON parser will now correctly reject certain bad inputs that it previously silently accepted (for example: json="{}x" was accepted).

PiperOrigin-RevId: 580493003
diff --git a/upb/json/BUILD b/upb/json/BUILD
@@ -41,6 +41,7 @@ cc_test(
         ":test_upb_proto",
         ":test_upb_proto_reflection",
         "@com_google_googletest//:gtest_main",
+        "//upb:base",
         "//upb:mem",
         "//upb:reflection",
     ],
diff --git a/upb/json/decode.c b/upb/json/decode.c
@@ -102,9 +102,13 @@ static bool jsondec_isvalue(const upb_FieldDef* f) {
          jsondec_isnullvalue(f);
 }
 
-UPB_NORETURN static void jsondec_err(jsondec* d, const char* msg) {
+static void jsondec_seterrmsg(jsondec* d, const char* msg) {
   upb_Status_SetErrorFormat(d->status, "Error parsing JSON @%d:%d: %s", d->line,
                             (int)(d->ptr - d->line_begin), msg);
+}
+
+UPB_NORETURN static void jsondec_err(jsondec* d, const char* msg) {
+  jsondec_seterrmsg(d, msg);
   UPB_LONGJMP(d->err, 1);
 }
 
@@ -119,7 +123,9 @@ UPB_NORETURN static void jsondec_errf(jsondec* d, const char* fmt, ...) {
   UPB_LONGJMP(d->err, 1);
 }
 
-static void jsondec_skipws(jsondec* d) {
+// Advances d->ptr until the next non-whitespace character or to the end of
+// the buffer.
+static void jsondec_consumews(jsondec* d) {
   while (d->ptr != d->end) {
     switch (*d->ptr) {
       case '\n':
@@ -135,7 +141,16 @@ static void jsondec_skipws(jsondec* d) {
         return;
     }
   }
-  jsondec_err(d, "Unexpected EOF");
+}
+
+// Advances d->ptr until the next non-whitespace character. Postcondition that
+// d->ptr is pointing at a valid non-whitespace character (will err if end of
+// buffer is reached).
+static void jsondec_skipws(jsondec* d) {
+  jsondec_consumews(d);
+  if (d->ptr == d->end) {
+    jsondec_err(d, "Unexpected EOF");
+  }
 }
 
 static bool jsondec_tryparsech(jsondec* d, char ch) {
@@ -1481,7 +1496,17 @@ static bool upb_JsonDecoder_Decode(jsondec* const d, upb_Message* const msg,
   if (UPB_SETJMP(d->err)) return false;
 
   jsondec_tomsg(d, msg, m);
-  return true;
+
+  // Consume any trailing whitespace before checking if we read the entire
+  // input.
+  jsondec_consumews(d);
+
+  if (d->ptr == d->end) {
+    return true;
+  } else {
+    jsondec_seterrmsg(d, "unexpected trailing characters");
+    return false;
+  }
 }
 
 bool upb_JsonDecode(const char* buf, size_t size, upb_Message* msg,
diff --git a/upb/json/decode_test.cc b/upb/json/decode_test.cc
@@ -30,10 +30,15 @@
 
 #include "upb/json/decode.h"
 
+#include <string>
+#include <vector>
+
 #include "google/protobuf/struct.upb.h"
 #include <gtest/gtest.h>
+#include "upb/base/status.hpp"
 #include "upb/json/test.upb.h"
 #include "upb/json/test.upbdefs.h"
+#include "upb/mem/arena.h"
 #include "upb/mem/arena.hpp"
 #include "upb/reflection/def.hpp"
 
@@ -100,3 +105,17 @@ TEST(JsonTest, DecodeConflictJsonName) {
   EXPECT_EQ(2, upb_test_Box_new_value(box));
   EXPECT_EQ(0, upb_test_Box_value(box));
 }
+
+TEST(JsonTest, RejectsBadTrailingCharacters) {
+  upb::Arena a;
+  std::string json_string = R"({}abc)";
+  upb_test_Box* box = JsonDecode(json_string.c_str(), a.ptr());
+  EXPECT_EQ(box, nullptr);
+}
+
+TEST(JsonTest, AcceptsTrailingWhitespace) {
+  upb::Arena a;
+  std::string json_string = "{} \n \r\n \t\t";
+  upb_test_Box* box = JsonDecode(json_string.c_str(), a.ptr());
+  EXPECT_NE(box, nullptr);
+}
diff --git a/upb/util/required_fields_test.cc b/upb/util/required_fields_test.cc
@@ -111,7 +111,7 @@ TYPED_TEST_SUITE(RequiredFieldsTest, MyTypes);
 // }
 TYPED_TEST(RequiredFieldsTest, TestRequired) {
   TestFixture::CheckRequired(R"json({})json", {"required_message"});
-  TestFixture::CheckRequired(R"json({"required_message": {}}")json", {});
+  TestFixture::CheckRequired(R"json({"required_message": {}})json", {});
   TestFixture::CheckRequired(
       R"json(
       {

Original file line number	Diff line number	Diff line change
`@@ -111,7 +111,7 @@ TYPED_TEST_SUITE(RequiredFieldsTest, MyTypes);`
`111`	`111`	`// }`
`112`	`112`	`TYPED_TEST(RequiredFieldsTest, TestRequired) {`
`113`	`113`	`TestFixture::CheckRequired(R"json({})json", {"required_message"});`
`114`		`- TestFixture::CheckRequired(R"json({"required_message": {}}")json", {});`
	`114`	`+ TestFixture::CheckRequired(R"json({"required_message": {}})json", {});`
`115`	`115`	`TestFixture::CheckRequired(`
`116`	`116`	`R"json(`
`117`	`117`	`{`