Bug#21823135 INVALID READ OF MEMORY FREED BY GIS_WKB_RAW_FREE

Issue:

The cmp_item_string::store_value() doesn't copy the string even if
value.is_alloced() is false, which means the string buffer referenced by 'value'
doesn't belong to 'value' but the cmp_item_string simply use this buffer
anyway, assuming the buffer will be always valid whenever it's accessed.
This is wrong in itself.

The way some GIS functions work is to return geometry blob buffer allocated
by Boost.Geometry without duplicating it, and free this buffer next time the
same function is called. Such behavior breaks above wrong assumption and
hence the memory issue.

Fix:

In cmp_item_string::store_value(), if 'value' 's referenced buffer was not
allocated by itself, duplicate its string.

Author: david-zhao

mysql-test/r/gis.result

@@ -2947,3 +2947,13 @@ ST_ISVALID(ST_INTERSECTION(ST_GEOMFROMTEXT('POLYGON((5 6,-15
 -13,1 -8,5 6))'), ST_GEOMFROMTEXT('POLYGON((0 8,-19 6,18 -17,20 8,11 17,0
 8),(3 2,3 -1,1 0,3 2),(1 3,4 4,0 -1,1 3))')))
 1
+#
+# Bug#21823135 INVALID READ OF MEMORY FREED BY GIS_WKB_RAW_FREE
+#
+SELECT point(1,1) IN ('1',1,'1') AS res;
+res
+0
+SELECT st_centroid(point(1,1)) IN ('1',1,'1') AS res;
+res
+0
+DO st_centroid(point(1,1)) IN ('1',1,'1');

mysql-test/t/gis.test

@@ -2771,3 +2771,11 @@ SELECT ST_ISVALID(ST_INTERSECTION(ST_GEOMFROMTEXT('POLYGON((0 5,-6
 SELECT ST_ISVALID(ST_INTERSECTION(ST_GEOMFROMTEXT('POLYGON((5 6,-15
 -13,1 -8,5 6))'), ST_GEOMFROMTEXT('POLYGON((0 8,-19 6,18 -17,20 8,11 17,0
 8),(3 2,3 -1,1 0,3 2),(1 3,4 4,0 -1,1 3))')));
+
+
+--echo #
+--echo # Bug#21823135 INVALID READ OF MEMORY FREED BY GIS_WKB_RAW_FREE
+--echo #
+SELECT point(1,1) IN ('1',1,'1') AS res;
+SELECT st_centroid(point(1,1)) IN ('1',1,'1') AS res;
+DO st_centroid(point(1,1)) IN ('1',1,'1');

sql-common/sql_string.cc

@@ -181,17 +181,37 @@ bool String::copy()
    before copying and the old buffer freed. Character set information is also
    copied.
 
+   If str is the same as this and str doesn't own its buffer, a
+   new buffer is allocated and it's owned by str.
+
    @param str The string whose internal buffer is to be copied.
 
    @retval false Success.
    @retval true Memory allocation failed.
 */
 bool String::copy(const String &str)
 {
+  /*
+    If &str == this and it owns the buffer, this operation is a no-op, so skip
+    the meaningless copy. Otherwise if we do, we will read freed memory at
+    the memmove call below.
+  */
+  if (&str == this && str.is_alloced())
+    return false;
+
+  /*
+    If a String s doesn't own its buffer, here we should allocate
+    a new buffer owned by s and copy the contents there. But alloc()
+    will change this->m_ptr and this->m_length, and if this == &str, this
+    will also change str->m_ptr and str->m_length, so we need to save
+    these values first.
+  */
+  const size_t str_length= str.m_length;
+  const char *str_ptr= str.m_ptr;
   if (alloc(str.m_length))
     return true;
-  m_length= str.m_length;
-  memmove(m_ptr, str.m_ptr, m_length);		// May be overlapping
+  m_length= str_length;
+  memmove(m_ptr, str_ptr, m_length);		// May be overlapping
   m_ptr[m_length]= 0;
   m_charset= str.m_charset;
   return false;

sql/item_cmpfunc.h

@@ -1429,7 +1429,7 @@ class cmp_item_string : public cmp_item_scalar
   virtual void store_value(Item *item)
   {
     String *res= item->val_str(&value);
-    if(res && (res != &value))
+    if (res && (res != &value || !res->is_alloced()))
     {
       // 'res' may point in item's transient internal data, so make a copy
       value.copy(*res);