MDEV-28598 Assertion 'got_name == named_item_expected()' failedbb-10.4-MDEV-28598

This assertion triggered on an attempt to write a piece of corrupt
JSON to the optimizer trace:
  "transformation": {
    "select_id": 2,
    "from": "IN (SELECT)",
    "to": "semijoin",
    {
       "join_optimization": {
       "select_id": 3,
       "steps": []
    }
  }

This happened because some parts of the query may be evaluated right
during the optimization stage. To handle such cases Json_writer will now
implicitly add named members with automatically assigned names
"implicitly_added_member_#1", "implicitly_added_member_#2", etc
to preserve JSON document correctness. Also the tail of the JSON document
will be printed to stderr before abort.
Those two improvements only apply for Debug builds, not for Release ones
as the Release builds skip JSON correctness checks

5 files changed, 219 insertions, 16 deletions

diff --git a/mysql-test/main/opt_trace.result b/mysql-test/main/opt_trace.result
index 34683d037ea..ada5a72b2fb 100644
--- a/mysql-test/main/opt_trace.result
+++ b/mysql-test/main/opt_trace.result
@@ -8647,5 +8647,15 @@ SELECT a FROM t1 WHERE (a,b) in (SELECT @c,@d);
 a
 DROP TABLE t1;
 #
+# MDEV-28598: Assertion `got_name == named_item_expected()' failed in Json_writer::on_start_object
+#
+CREATE TABLE t1 (a int);
+SET optimizer_trace=1;
+INSERT INTO t1 VALUES (1),(2),(3);
+SELECT * FROM t1 WHERE a IN (SELECT 1 a FROM t1 WHERE EXISTS (select 1));
+a
+1
+DROP TABLE t1;
+#
 # End of 10.4 tests
 #
diff --git a/mysql-test/main/opt_trace.test b/mysql-test/main/opt_trace.test
index 855ce11aaf5..ab30b23b743 100644
--- a/mysql-test/main/opt_trace.test
+++ b/mysql-test/main/opt_trace.test
@@ -654,5 +654,15 @@ SELECT a FROM t1 WHERE (a,b) in (SELECT @c,@d);
 DROP TABLE t1;
 
 --echo #
+--echo # MDEV-28598: Assertion `got_name == named_item_expected()' failed in Json_writer::on_start_object
+--echo #
+
+CREATE TABLE t1 (a int);
+SET optimizer_trace=1;
+INSERT INTO t1 VALUES (1),(2),(3);
+SELECT * FROM t1 WHERE a IN (SELECT 1 a FROM t1 WHERE EXISTS (select 1));
+DROP TABLE t1;
+
+--echo #
 --echo # End of 10.4 tests
 --echo #
diff --git a/sql/my_json_writer.cc b/sql/my_json_writer.cc
index 8e35b25b822..c95e4c6d657 100644
--- a/sql/my_json_writer.cc
+++ b/sql/my_json_writer.cc
@@ -39,7 +39,7 @@ inline void Json_writer::on_start_object()
 #if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
   if(!fmt_helper.is_making_writer_calls())
   {
-    VALIDITY_ASSERT(got_name == named_item_expected());
+    validate_named_item_expectation(true);
     named_items_expectation.push_back(true);
   }
 #endif
@@ -68,7 +68,7 @@ void Json_writer::start_array()
 #if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
   if(!fmt_helper.is_making_writer_calls())
   {
-    VALIDITY_ASSERT(got_name == named_item_expected());
+    validate_named_item_expectation(true);
     named_items_expectation.push_back(false);
     got_name= false;
   }
@@ -91,9 +91,20 @@ void Json_writer::start_array()
 void Json_writer::end_object()
 {
 #if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
-  VALIDITY_ASSERT(named_item_expected());
+  if (!named_item_expected())
+  {
+    print_json_tail_to_stderr(
+          "end_object failed since an unnamed item is expected");
+    VALIDITY_ASSERT(false);
+  }
   named_items_expectation.pop_back();
-  VALIDITY_ASSERT(!got_name);
+
+  if (got_name)
+  {
+    print_json_tail_to_stderr(
+          "end_object failed since got_name==true");
+    VALIDITY_ASSERT(false);
+  }
   got_name= false;
 #endif
   indent_level-=INDENT_SIZE;
@@ -107,7 +118,12 @@ void Json_writer::end_object()
 void Json_writer::end_array()
 {
 #if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
-  VALIDITY_ASSERT(!named_item_expected());
+  if (named_item_expected())
+  {
+    print_json_tail_to_stderr(
+          "end_array failed since a named item is expected");
+    VALIDITY_ASSERT(false);
+  }
   named_items_expectation.pop_back();
   got_name= false;
 #endif
@@ -130,8 +146,18 @@ Json_writer& Json_writer::add_member(const char *name, size_t len)
 {
   if (!fmt_helper.on_add_member(name, len))
   {
+#if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
     // assert that we are in an object
-    DBUG_ASSERT(!element_started);
+    if (element_started)
+    {
+      char err_descr[200];
+      snprintf(err_descr, sizeof(err_descr),
+               "failed to add_member(\"%s\", %zu) since element_started=false",
+               name, len);
+      print_json_tail_to_stderr(err_descr);
+      VALIDITY_ASSERT(false);
+    }
+#endif
     start_element();
 
     output.append('"');
@@ -242,8 +268,12 @@ void Json_writer::add_unquoted_str(const char* str)
 
 void Json_writer::add_unquoted_str(const char* str, size_t len)
 {
-  VALIDITY_ASSERT(fmt_helper.is_making_writer_calls() ||
-                  got_name == named_item_expected());
+#if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
+  if (!fmt_helper.is_making_writer_calls())
+  {
+    validate_named_item_expectation();
+  }
+#endif
   if (on_add_str(str, len))
     return;
 
@@ -275,8 +305,12 @@ void Json_writer::add_str(const char *str)
 
 void Json_writer::add_str(const char* str, size_t num_bytes)
 {
-  VALIDITY_ASSERT(fmt_helper.is_making_writer_calls() ||
-                  got_name == named_item_expected());
+#if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
+  if (!fmt_helper.is_making_writer_calls())
+  {
+    validate_named_item_expectation();
+  }
+#endif
   if (on_add_str(str, num_bytes))
     return;
 
@@ -294,6 +328,53 @@ void Json_writer::add_str(const String &str)
   add_str(str.ptr(), str.length());
 }
 
+#if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
+void Json_writer::print_json_tail_to_stderr(const char *err_descr)
+{
+  size_t pos_to_print_from=
+    output.get_string()->length() < MAX_JSON_LEN_FOR_STDERR ?
+      0 : output.get_string()->length() - MAX_JSON_LEN_FOR_STDERR;
+  auto str_to_print= output.get_string()->c_ptr() + pos_to_print_from;
+  fprintf(stderr, "JSON writer error: %s.\nJSON tail: %s\n",
+          err_descr, str_to_print);
+}
+
+void Json_writer::validate_named_item_expectation(bool fix_wth_implicit_member)
+{
+  if (got_name != named_item_expected())
+  {
+    if (!got_name && fix_wth_implicit_member)
+    {
+      /*
+        Context is expecting a named member but has an unnamed one.
+        Add a member with automatically assigned name to preserve
+        JSON document correctness
+      */
+      add_implicit_member();
+      return;
+    }
+    const char *expected= named_item_expected() ?
+          "named" : "unnamed";
+    const char *actual= named_item_expected() ?
+          "unnamed" : "named";
+    char err_descr[100];
+    snprintf(err_descr, sizeof(err_descr),
+             "expected a %s member but a %s one has been added",
+             expected, actual);
+    print_json_tail_to_stderr(err_descr);
+    VALIDITY_ASSERT(false);
+  }
+}
+
+void Json_writer::add_implicit_member()
+{
+  char name[50];
+  sprintf(name, "implicitly_added_member_#%u", implicit_member_num);
+  add_member(name);
+  implicit_member_num++;
+}
+#endif
+
 Json_writer_temp_disable::Json_writer_temp_disable(THD *thd_arg)
 {
   thd= thd_arg;
@@ -308,6 +389,7 @@ bool Single_line_formatting_helper::on_add_member(const char *name,
                                                   size_t len)
 {
   DBUG_ASSERT(state== INACTIVE || state == DISABLED);
+
   if (state != DISABLED)
   {
     // remove everything from the array
diff --git a/sql/my_json_writer.h b/sql/my_json_writer.h
index 4b69ba70675..d6b8ade091f 100644
--- a/sql/my_json_writer.h
+++ b/sql/my_json_writer.h
@@ -189,7 +189,7 @@ public:
       str.append(c);
   }
 
-  const String *get_string() { return &str; }
+  String *get_string() { return &str; }
   size_t length() { return str.length(); }
 private:
   String str;
@@ -296,6 +296,56 @@ private:
   void start_element();
   void start_sub_element();
 
+#if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
+  /*
+    Prints the tail of JSON composed so far to stderr.
+    Output is limited by MAX_JSON_LEN_FOR_STDERR characters.
+    Format is as follows:
+    "JSON writer error: <description of the error>.
+    JSON tail: <...JSON...>
+  */
+  void print_json_tail_to_stderr(const char *err_descr);
+
+  enum { MAX_JSON_LEN_FOR_STDERR = 1000 };
+
+  /*
+    If the context of the JSON document is expecting a unnamed member
+    but a named one is added this will trigger an assertion failure.
+    The same works vice versa unless the "fix_wth_implicit_member"
+    flag is set. When fix_wth_implicit_member == true and the context is
+    expecting a named member but has got an unnamed one,
+    the function will implicitly add a JSON member and automatically assign
+    its name. In this case correctness of JSON document is preserved
+    and no assertion will trigger.
+    Example:
+      transformation": {
+        "select_id": 2,
+        {
+          "join_optimization": {
+            "select_id": 3,
+            "steps": []
+        }
+
+     will be converted to
+      transformation": {
+        "select_id": 2,
+        "implicitly_added_member_#1": {
+          "join_optimization": {
+            "select_id": 3,
+            "steps": []
+        }
+  */
+  void validate_named_item_expectation(bool fix_wth_implicit_member= false);
+
+  void add_implicit_member();
+
+  /*
+    This incremental counter is used to automatically assign names
+    for implicitly added members
+  */
+  unsigned int implicit_member_num= 1;
+#endif
+
 public:
   String_with_limit output;
 };
diff --git a/unittest/sql/my_json_writer-t.cc b/unittest/sql/my_json_writer-t.cc
index 6398a589c33..e05dc4836ea 100644
--- a/unittest/sql/my_json_writer-t.cc
+++ b/unittest/sql/my_json_writer-t.cc
@@ -65,8 +65,43 @@ int main(int args, char **argv)
   {
     Json_writer w;
     w.start_object();
+    w.add_ll(123678456);
+    ok(w.invalid_json, "Unnamed LL value in an object");
+  }
+
+  {
+    Json_writer w;
+    w.start_object();
     w.add_ull(123);
-    ok(w.invalid_json, "Unnamed value in an object");
+    ok(w.invalid_json, "Unnamed ULL value in an object");
+  }
+
+  {
+    Json_writer w;
+    w.start_object();
+    w.add_size(567890);
+    ok(w.invalid_json, "Unnamed size value in an object");
+  }
+
+  {
+    Json_writer w;
+    w.start_object();
+    w.add_double(123.567867);
+    ok(w.invalid_json, "Unnamed double value in an object");
+  }
+
+  {
+    Json_writer w;
+    w.start_object();
+    w.add_bool(true);
+    ok(w.invalid_json, "Unnamed bool value in an object");
+  }
+
+  {
+    Json_writer w;
+    w.start_object();
+    w.add_null();
+    ok(w.invalid_json, "Unnamed null value in an object");
   }
 
   {
@@ -79,15 +114,33 @@ int main(int args, char **argv)
   {
     Json_writer w;
     w.start_object();
+    w.add_str("some string");
+    ok(w.invalid_json, "Unnamed string (const char*) in an object");
+  }
+
+  {
+    Json_writer w;
+    w.start_object();
+    CHARSET_INFO cs_info{};
+    String str("another string", &cs_info);
+    w.add_str(str);
+    ok(w.invalid_json, "Unnamed string (class String) in an object");
+  }
+
+  {
+    Json_writer w;
+    w.start_object();
     w.start_array();
-    ok(w.invalid_json, "Unnamed array in an object");
+    ok(!w.invalid_json, "Implicitly added member for the unnamed array"
+                        " in an object");
   }
 
   {
     Json_writer w;
     w.start_object();
     w.start_object();
-    ok(w.invalid_json, "Unnamed object in an object");
+    ok(!w.invalid_json, "Implicitly added member for the unnamed object"
+                        " in an object");
   }
 
   {
@@ -119,8 +172,6 @@ int main(int args, char **argv)
     ok(w.invalid_json, "JSON array end of object");
   }
 
-
-
   diag("Done");
 
   return exit_status();