Bug#21547877: UPDATE/INSERT JSON COLUMN CRASHES IF EXPRESSION REFERS TO SELF

Problem: If a multi-column update statement fails when updating one of
the columns in a row, it will go on and update the remaining columns
in that row before it stops and reports an error. If the failure
happens when updating a JSON column, and the JSON column is also
referenced later in the update statement, new and more serious errors
can happen when the update statement attempts to read the JSON column,
as it may contain garbage at this point.

The fix is twofold:

1) Field_json::val_str() currently returns NULL if an error happens.
This is correct for val_str() functions in the Item class hierarchy,
but not for val_str() functions in the Field class hierarchy. The
val_str() functions in the Field classes instead return a pointer to
an empty String object on error. Since callers don't expect it to
return NULL, this caused a crash when a caller unconditionally
dereferenced the returned pointer. The patch makes
Field_json::val_str() return a pointer to an empty String on error to
avoid such crashes.

2) Whereas #1 fixes the immediate crash, Field_json::val_str() may
still read garbage when this situation occurs. This could lead to
unreliable behaviour, and both valgrind and ASAN warn about it. The
patch therefore also makes Field_json::store() start by clearing the
field, so that it will hold an empty value rather than garbage after
an error has happened.

Fix #2 is sufficient to fix the reported problems. Fix #1 is included
for consistency, so that Field_json::val_str() behaves the same way as
the other Field::val_str() functions.

The query in the bug report didn't always crash. Since the root cause
was that it had read garbage, it could be lucky and read something
that looked like a valid value. In that case, Field_json::val_str()
didn't return NULL, and the crash was avoided.

The patch also makes these changes:

- It removes the Field_json::store_dom() function, since it is only
  called from one place. It is now inlined instead.

- It corrects information about return values in the comment that
  describes the ensure_utf8mb4() function.

Author: kahatlen

mysql-test/r/json.result

@@ -13267,3 +13267,16 @@ select row(uuid(), a) < row(a, str_to_date(1,1)) from t;
 row(uuid(), a) < row(a, str_to_date(1,1))
 1
 drop table t;
+# Bug#21547877: UPDATE/INSERT JSON COLUMN CRASHES IF EXPRESSION
+#               REFERS TO SELF
+#
+SET NAMES latin1;
+CREATE TABLE t (j JSON);
+INSERT INTO t VALUES ('{}');
+UPDATE t SET j='1', j='1111-11-11', j=('1' NOT BETWEEN j AND '1');
+ERROR 22032: Invalid JSON text: "The document root must not follow by other values." at position 4 in value (or column) '1111-11-11'.
+SELECT * FROM t;
+j
+{}
+DROP TABLE t;
+SET NAMES DEFAULT;

mysql-test/t/json.test

@@ -6043,6 +6043,17 @@ create table t(a json not null) engine=innodb;
 insert into t values('{}');
 select row(uuid(), a) < row(a, str_to_date(1,1)) from t;
 drop table t;
+--echo # Bug#21547877: UPDATE/INSERT JSON COLUMN CRASHES IF EXPRESSION
+--echo #               REFERS TO SELF
+--echo #
+SET NAMES latin1;
+CREATE TABLE t (j JSON);
+INSERT INTO t VALUES ('{}');
+--error ER_INVALID_JSON_TEXT
+UPDATE t SET j='1', j='1111-11-11', j=('1' NOT BETWEEN j AND '1');
+SELECT * FROM t;
+DROP TABLE t;
+SET NAMES DEFAULT;
 
 # Local Variables:
 # mode: sql

sql/field.cc

@@ -8717,7 +8717,7 @@ uint Field_json::is_equal(Create_field *new_field)
 /**
   Store data in this JSON field.
 
-  JSON data is usually stored using store_dom() or store_json(), so this
+  JSON data is usually stored using store(Field_json*) or store_json(), so this
   function will only be called if non-JSON data is attempted stored in a JSON
   field. This results in an error in most cases.
 
@@ -8746,6 +8746,14 @@ Field_json::store(const char *from, size_t length, const CHARSET_INFO *cs)
 {
   ASSERT_COLUMN_MARKED_FOR_WRITE;
 
+  /*
+    First clear the field so that it doesn't contain garbage if we
+    return with an error. Some callers continue for a while even after
+    an error has been raised, and they could get into trouble if the
+    field contains garbage.
+  */
+  reset();
+
   const char *s;
   size_t ss;
   String v(from, length, cs);
@@ -8770,22 +8778,7 @@ Field_json::store(const char *from, size_t length, const CHARSET_INFO *cs)
     return TYPE_ERR_BAD_VALUE;
   }
 
-  return store_dom(dom.get());
-}
-
-
-/**
-  Convert a Json_dom object to binary representation and store it in this
-  field.
-
-  @param dom the Json_dom to store
-  @return zero on success, non-zero on failure
-*/
-type_conversion_status Field_json::store_dom(const Json_dom *dom)
-{
-  ASSERT_COLUMN_MARKED_FOR_WRITE;
-
-  if (json_binary::serialize(dom, &value))
+  if (json_binary::serialize(dom.get(), &value))
     return TYPE_ERR_BAD_VALUE;
 
   return store_binary(value.ptr(), value.length());
@@ -8986,19 +8979,17 @@ String *Field_json::val_str(String *buf1, String *buf2 __attribute__((unused)))
 {
   ASSERT_COLUMN_MARKED_FOR_READ;
 
-  Json_wrapper wr;
-  if (is_null() || val_json(&wr))
-    return NULL;
-
   /*
     Per contract of Field::val_str(String*,String*), buf1 should be
     used if the value needs to be converted to string, and buf2 should
     be used if the string value is already known. We need to convert,
     so use buf1.
   */
   buf1->length(0);
-  if (wr.to_string(buf1, true, field_name))
-    return NULL;                                /* purecov: inspected */
+
+  Json_wrapper wr;
+  if (is_null() || val_json(&wr) || wr.to_string(buf1, true, field_name))
+    buf1->length(0);
 
   return buf1;
 }

sql/field.h

@@ -3907,7 +3907,6 @@ class Field_geom :public Field_blob {
 class Field_json :public Field_blob
 {
   type_conversion_status unsupported_conversion();
-  type_conversion_status store_dom(const Json_dom *dom);
   type_conversion_status store_binary(const char *ptr, size_t length);
 public:
   Field_json(uchar *ptr_arg, uchar *null_ptr_arg, uint null_bit_arg,

sql/item_json_func.h

@@ -222,8 +222,8 @@ bool get_json_atom_wrapper(Item **args, uint arg_idx,
 /**
   Check a non-empty val for character set. If it has character set
   my_charset_binary, signal error and return false. Else, try to convert to
-  my_charset_utf8mb4_binary. If this fails, signal error and return false, else
-  return true.
+  my_charset_utf8mb4_binary. If this fails, signal error and return true, else
+  return false.
 
   @param[in]     val       the string to be checked
   @param[in,out] buf       buffer to hold the converted string