MDEV-39720: Optimizer Trace doesn't quote string values, may have inv…

mysql-test/main/explain_json.result

@@ -1477,7 +1477,7 @@ EXPLAIN
           "rows": 2,
           "cost": "COST_REPLACED",
           "filtered": 100,
-          "attached_condition": "t1.a = _latin1'\xDF'"
+          "attached_condition": "t1.a = _latin1'\\xDF'"
         }
       }
     ]

mysql-test/main/opt_trace.result

@@ -10526,7 +10526,7 @@ JS
             {
                 "index": "i_b",
                 "ranges": 
-                ["(\xD9[\x943j\x99F\xA3\x9C\xF5\xB5\x8C\xFEw-\x8C) <= (b) <= (\xD9[\x943j\x99F\xA3\x9C\xF5\xB5\x8C\xFEw-\x8C)"],
+                ["(\\xD9[\\x943j\\x99F\\xA3\\x9C\\xF5\\xB5\\x8C\\xFEw-\\x8C) <= (b) <= (\\xD9[\\x943j\\x99F\\xA3\\x9C\\xF5\\xB5\\x8C\\xFEw-\\x8C)"],
                 "rowid_ordered": true,
                 "using_mrr": false,
                 "index_only": false,
@@ -10588,7 +10588,7 @@ JS
             {
                 "index": "i_b",
                 "ranges": 
-                ["(ab\x0A) <= (b) <= (ab\x0A)"],
+                ["(ab\\x0A) <= (b) <= (ab\\x0A)"],
                 "rowid_ordered": true,
                 "using_mrr": false,
                 "index_only": false,
@@ -10616,7 +10616,7 @@ JS
             {
                 "index": "i_b",
                 "ranges": 
-                ["(ab\x0A\x00\x00\x00\x00\x00\x00\x00) <= (b) <= (ab\x0A\x00\x00\x00\x00\x00\x00\x00)"],
+                ["(ab\\x0A\\x00\\x00\\x00\\x00\\x00\\x00\\x00) <= (b) <= (ab\\x0A\\x00\\x00\\x00\\x00\\x00\\x00\\x00)"],
                 "rowid_ordered": true,
                 "using_mrr": false,
                 "index_only": false,
@@ -10644,7 +10644,7 @@ JS
             {
                 "index": "i_b",
                 "ranges": 
-                ["(ab\x0A) <= (b) <= (ab\x0A)"],
+                ["(ab\\x0A) <= (b) <= (ab\\x0A)"],
                 "rowid_ordered": true,
                 "using_mrr": false,
                 "index_only": false,
@@ -10675,7 +10675,7 @@ JS
             {
                 "index": "i_b",
                 "ranges": 
-                ["(ab\n) <= (b) <= (ab\n)"],
+                ["(ab\\n) <= (b) <= (ab\\n)"],
                 "rowid_ordered": true,
                 "using_mrr": false,
                 "index_only": false,
@@ -10709,7 +10709,7 @@ JS
             {
                 "index": "i_b",
                 "ranges": 
-                ["(ab\x0A) <= (b) <= (ab\x0A)"],
+                ["(ab\\x0A) <= (b) <= (ab\\x0A)"],
                 "rowid_ordered": false,
                 "using_mrr": false,
                 "index_only": false,
@@ -10741,7 +10741,7 @@ JS
             {
                 "index": "i_b",
                 "ranges": 
-                ["(ab\n) <= (b) <= (ab\n)"],
+                ["(ab\\n) <= (b) <= (ab\\n)"],
                 "rowid_ordered": true,
                 "using_mrr": false,
                 "index_only": false,
@@ -12968,7 +12968,7 @@ x
 𝄞
 SELECT json_valid(trace) AS valid_json, json_detailed(json_extract(trace,'$**.expanded_query')) FROM information_schema.optimizer_trace;
 valid_json	json_detailed(json_extract(trace,'$**.expanded_query'))
-1	["Error: failed to escape query string for JSON output"]
+1	["select '𝄞' AS x"]
 SET NAMES DEFAULT;
 set optimizer_trace=@saved_optimizer_trace;
 # End of 10.11 tests
@@ -13888,6 +13888,22 @@ OPTIMIZER_TRACE	CREATE TEMPORARY TABLE `OPTIMIZER_TRACE` (
   `MISSING_BYTES_BEYOND_MAX_MEM_SIZE` int(20) NOT NULL,
   `INSUFFICIENT_PRIVILEGES` tinyint(1) NOT NULL
 ) ENGINE=Aria DEFAULT CHARSET=utf8mb3 COLLATE=utf8mb3_general_ci PAGE_CHECKSUM=0
+#
+# MDEV-39720: Optimizer Trace may have invalid JSON due to quoting.
+#
+create table t1 (
+a varchar(100)
+);
+set optimizer_trace=1;
+explain
+select * from t1 where a < '"';
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	NULL	NULL	NULL	NULL	NULL	NULL	NULL	Impossible WHERE noticed after reading const tables
+set @trace=(select trace from information_schema.optimizer_trace);
+select json_valid(@trace);
+json_valid(@trace)
+1
+drop table t1;
 set  @@optimizer_switch= @save_optimizer_switch;
 set  @@use_stat_tables= @save_use_stat_tables;
 set  @@histogram_size= @save_histogram_size;

mysql-test/main/opt_trace.test

@@ -1389,6 +1389,22 @@ drop table t1;
 
 SHOW CREATE TABLE information_schema.optimizer_trace;
 
+--echo #
+--echo # MDEV-39720: Optimizer Trace may have invalid JSON due to quoting.
+--echo #
+create table t1 (
+  a varchar(100)
+);
+
+set optimizer_trace=1;
+explain
+select * from t1 where a < '"';
+
+set @trace=(select trace from information_schema.optimizer_trace);
+select json_valid(@trace);
+
+drop table t1;
+
 set  @@optimizer_switch= @save_optimizer_switch;
 set  @@use_stat_tables= @save_use_stat_tables;
 set  @@histogram_size= @save_histogram_size;

sql/my_json_writer.cc

@@ -14,6 +14,7 @@
    Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */
 
 #include "my_global.h"
+#include <json_lib.h>
 #include "my_json_writer.h"
 
 #if !defined(NDEBUG) || defined(JSON_WRITER_UNIT_TEST)
@@ -284,10 +285,45 @@ void Json_writer::add_str(const char *str)
 }
 
 /*
-  This function is used to add only num_bytes of str to the output string
+  @brief
+    Add a string pointed by str of num_bytes length, escaping it if needed.
+
+  @param
+    str         String in utf8mb4.
+    num_bytes   Length of string in bytes (not characters).
 */
 
 void Json_writer::add_str(const char* str, size_t num_bytes)
+{
+  int rc;
+  /* Initial buffer, json_escape_to_string will alloc larger if needed */
+  StringBuffer<128> buf;
+  if ((rc= json_escape_to_string(str, num_bytes, &my_charset_utf8mb4_bin, &buf)))
+  {
+    if (rc == JSON_ERROR_ILLEGAL_SYMBOL)
+      str= "String with illegal unicode symbol";
+    else
+    {
+      DBUG_ASSERT(rc == JSON_ERROR_OUT_OF_SPACE);
+      str= "Out of memory writing JSON";
+    }
+    num_bytes= strlen(str);
+  }
+  else
+  {
+    str= buf.ptr();
+    num_bytes= buf.length();
+  }
+  add_escaped_str(str, num_bytes);
+}
+
+
+/*
+  @brief
+    Add a string value. The caller guarantees that it doesn't need escaping.
+*/
+
+void Json_writer::add_escaped_str(const char* str, size_t num_bytes)
 {
   VALIDITY_ASSERT(fmt_helper.is_making_writer_calls() ||
                   got_name == named_item_expected());
@@ -305,7 +341,22 @@ void Json_writer::add_str(const char* str, size_t num_bytes)
 
 void Json_writer::add_str(const String &str)
 {
-  add_str(str.ptr(), str.length());
+  uint32 offs;
+  /* str may be in any charset. Convert to utf8mb4 if necessary */
+  if (String::needs_conversion(str.length(), str.charset(),
+                               &my_charset_utf8mb4_bin, &offs))
+  {
+    uint err;
+    StringBuffer<128> converted_str;
+    if (converted_str.copy(&str, &my_charset_utf8mb4_bin, &err))
+    {
+      const char *errstr= "Conversion error writing JSON";
+      converted_str.set(errstr, strlen(errstr), &my_charset_utf8mb4_bin);
+    }
+    add_str(converted_str.ptr(), converted_str.length());
+  }
+  else
+    add_str(str.ptr(), str.length());
 }
 
 #ifdef ENABLED_JSON_WRITER_CONSISTENCY_CHECKS
@@ -493,3 +544,55 @@ void Single_line_formatting_helper::disable_and_flush()
   buf_ptr= buffer;
   state= INACTIVE;
 }
+
+/*
+  @brief
+    Escape a JSON string and save it into *out.
+
+  @detail
+    There's no way to tell how much space is needed for the output.
+    Start with a small string and increase its size until json_escape()
+    succeeds.
+*/
+
+int json_escape_to_string(const char *str, size_t len, CHARSET_INFO *cs,
+                          String *out)
+{
+  /* Safety: assert that str doesn't point into *out. */
+  DBUG_ASSERT(!out->alloced_length() ||
+              str < out->ptr() || str >= out->ptr() + out->alloced_length());
+
+  // Make sure 'out' has some memory allocated.
+  if (!out->alloced_length() && out->alloc(128))
+    return JSON_ERROR_OUT_OF_SPACE;
+
+  while (1)
+  {
+    uchar *buf= (uchar*)out->ptr();
+    out->length(out->alloced_length());
+    const uchar *str_ptr= (const uchar*)str;
+
+    int res= json_escape(cs,
+                         str_ptr,
+                         str_ptr + len,
+                         &my_charset_utf8mb4_bin,
+                         buf, buf + out->length());
+    if (res >= 0)
+    {
+      out->length(res);
+      return 0; // Ok
+    }
+
+    if (res != JSON_ERROR_OUT_OF_SPACE)
+      return res; // Some conversion error
+
+    // Out of space error. Try with a bigger buffer
+    if (out->alloc(out->alloced_length()*2))
+      return JSON_ERROR_OUT_OF_SPACE;
+  }
+}
+
+int json_escape_to_string(const String *str, String *out)
+{
+  return json_escape_to_string(str->ptr(), str->length(), str->charset(), out);
+}

sql/my_json_writer.h

@@ -243,11 +243,14 @@ class Json_writer
 
   /* Add atomic values */
 
-  /* Note: the add_str methods do not do escapes. Should this change? */
+  /* All const char* arguments are strings in utf8mb4 */
   void add_str(const char* val);
   void add_str(const char* val, size_t num_bytes);
   void add_str(const String &str);
   void add_str(Item *item);
+
+  /* Add a string for which the caller has done escaping needed in JSON */
+  void add_escaped_str(const char* val, size_t len);
   void add_table_name(const JOIN_TAB *tab);
   void add_table_name(const TABLE* table);
 
@@ -798,4 +801,8 @@ class Json_writer_nesting_guard
 #endif
 };
 
+int json_escape_to_string(const String *str, String *out);
+int json_escape_to_string(const char *str, size_t len, CHARSET_INFO *cs,
+                          String *out);
+
 #endif

sql/opt_histogram_json.cc

@@ -67,49 +67,6 @@ bool json_unescape_to_string(const char *val, int val_len, String* out)
 }
 
 
-/*
-  @brief
-    Escape a JSON string and save it into *out.
-
-  @detail
-    There's no way to tell how much space is needed for the output.
-    Start with a small string and increase its size until json_escape()
-    succeeds.
-*/
-
-static int json_escape_to_string(const String *str, String* out)
-{
-  // Make sure 'out' has some memory allocated.
-  if (!out->alloced_length() && out->alloc(128))
-    return JSON_ERROR_OUT_OF_SPACE;
-
-  while (1)
-  {
-    uchar *buf= (uchar*)out->ptr();
-    out->length(out->alloced_length());
-    const uchar *str_ptr= (const uchar*)str->ptr();
-
-    int res= json_escape(str->charset(),
-                         str_ptr,
-                         str_ptr + str->length(),
-                         &my_charset_utf8mb4_bin,
-                         buf, buf + out->length());
-    if (res >= 0)
-    {
-      out->length(res);
-      return 0; // Ok
-    }
-
-    if (res != JSON_ERROR_OUT_OF_SPACE)
-      return res; // Some conversion error
-
-    // Out of space error. Try with a bigger buffer
-    if (out->alloc(out->alloced_length()*2))
-      return JSON_ERROR_OUT_OF_SPACE;
-  }
-}
-
-
 class Histogram_json_builder : public Histogram_builder
 {
   Histogram_json_hb *histogram;
@@ -256,7 +213,7 @@ class Histogram_json_builder : public Histogram_builder
       if (!rc)
       {
         writer.add_member(is_start? "start": "end");
-        writer.add_str(escaped_val.c_ptr_safe());
+        writer.add_escaped_str(escaped_val.ptr(), escaped_val.length());
         return false;
       }
     }

sql/opt_trace.cc

@@ -128,18 +128,7 @@ void opt_trace_print_expanded_query(THD *thd, SELECT_LEX *select_lex,
     The output is not very pretty lots of back-ticks, the output
     is as the one in explain extended , lets try to improved it here.
   */
-
-  StringBuffer<1024> escaped_str(system_charset_info);
-  if (st_append_escaped(&escaped_str, &str) == 0)
-  {
-    writer->add("expanded_query", escaped_str.c_ptr_safe(), 
-                                  escaped_str.length());
-  }
-  else
-  {
-    writer->add("expanded_query", 
-      "Error: failed to escape query string for JSON output");
-  }
+  writer->add("expanded_query", str.c_ptr_safe(), str.length());
 }
 
 void opt_trace_disable_if_no_security_context_access(THD *thd)

sql/opt_trace_ddl_info.cc

@@ -80,28 +80,7 @@ static bool dump_record_to_trace(THD *thd, DDL_Key *ddl_key, String *stmt)
 {
   Json_writer_object ddl_wrapper(thd);
   ddl_wrapper.add("name", ddl_key->name);
-  size_t non_esc_stmt_len= stmt->length();
-  /*
-    making escape_stmt size to be 4 times the non_esc_stmt
-    4 is chosen as a worst case although 3 should suffice.
-    "'" would be escaped to \"\'\"
-  */
-  size_t len_multiplier= sizeof(uint32_t);
-  size_t escape_stmt_len= len_multiplier * non_esc_stmt_len;
-  char *escaped_stmt= (char *) thd->alloc(escape_stmt_len + 1);
-
-  if (!escaped_stmt)
-    return true;
-
-  int act_escape_stmt_len=
-      json_escape_string(stmt->c_ptr(), stmt->c_ptr() + non_esc_stmt_len,
-                         escaped_stmt, escaped_stmt + escape_stmt_len);
-
-  if (act_escape_stmt_len < 0)
-    return true;
-
-  escaped_stmt[act_escape_stmt_len]= 0;
-  ddl_wrapper.add("ddl", escaped_stmt);
+  ddl_wrapper.add("ddl", stmt->c_ptr_safe());
   return false;
 }