bzr commit into mysql-5.0-bugteam branch (gshchepa:2714) Bug#38816

[Gleb Shchepa]

#At file:///work/bzr/5.0-bugteam-38816/

 2714 Gleb Shchepa	2008-11-12
      Bug #38816: kill + flush tables with read lock + stored
                  procedures causes crashes!
      
      The problem of that bugreport was mostly fixed by the
      patch for bug 38691.
      However, attached test case focused on another crash or
      valgrind warning problem: SHOW PROCESSLIST query accesses
      freed memory of SP instruction that run in a parallel
      connection.
      
      Changes of thd->query/thd->query_length inside
      sp_instr_stmt::execute have been guarded with the
      LOCK_thread_cound mutex.
modified:
  sql/sp_head.cc

per-file messages:
  sql/sp_head.cc
    1. Changes of thd->query{_length} inside
    sp_instr_stmt::execute and subst_spvars functions have been
    guarded with LOCK_thread_cound.
    2. sp_instr_stmt::execute: call to alloc_query() has been
    removed to eliminate unsafe change of thd->query{_length}
    and unnecessary memory allocation (that may be ignored and
    allocated again in subst_spvars()).
    3. The subst_spvars function has been changed to allocate
    thd->query always.
=== modified file 'sql/sp_head.cc'
--- a/sql/sp_head.cc	2008-10-02 08:10:06 +0000
+++ b/sql/sp_head.cc	2008-11-12 10:42:30 +0000
@@ -870,6 +870,8 @@ subst_spvars(THD *thd, sp_instr *instr, 
   char *pbuf, *cur, buffer[512];
   String qbuf(buffer, sizeof(buffer), &my_charset_bin);
   int prev_pos, res, buf_len;
+  const char *query;
+  uint32 query_length;
 
   /* Find all instances of Item_splocal used in this statement */
   for (Item *item= instr->free_list; item; item= item->next)
@@ -882,7 +884,11 @@ subst_spvars(THD *thd, sp_instr *instr, 
     }
   }
   if (!sp_vars_uses.elements())
-    DBUG_RETURN(FALSE);
+  {
+    query= query_str->str;
+    query_length= query_str->length;
+    goto done;
+  }
     
   /* Sort SP var refs by their occurences in the query */
   sp_vars_uses.sort(cmp_splocal_locations);
@@ -932,21 +938,27 @@ subst_spvars(THD *thd, sp_instr *instr, 
   if (res)
     DBUG_RETURN(TRUE);
 
+  query_length= qbuf.length();
+  query= qbuf.ptr();
+
   /*
     Allocate additional space at the end of the new query string for the
     query_cache_send_result_to_client function.
   */
-  buf_len= qbuf.length() + thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE + 1;
+done:
+  buf_len= query_length + thd->db_length + 1 + QUERY_CACHE_FLAGS_SIZE + 1;
   if ((pbuf= alloc_root(thd->mem_root, buf_len)))
   {
-    memcpy(pbuf, qbuf.ptr(), qbuf.length());
-    pbuf[qbuf.length()]= 0;
+    memcpy(pbuf, query, query_length);
+    pbuf[query_length]= 0;
   }
   else
     DBUG_RETURN(TRUE);
 
+  VOID(pthread_mutex_lock(&LOCK_thread_count));
   thd->query= pbuf;
-  thd->query_length= qbuf.length();
+  thd->query_length= query_length;
+  VOID(pthread_mutex_unlock(&LOCK_thread_count));
 
   DBUG_RETURN(FALSE);
 }
@@ -2604,8 +2616,7 @@ sp_instr_stmt::execute(THD *thd, uint *n
 
   query= thd->query;
   query_length= thd->query_length;
-  if (!(res= alloc_query(thd, m_query.str, m_query.length+1)) &&
-      !(res=subst_spvars(thd, this, &m_query)))
+  if (!(res=subst_spvars(thd, this, &m_query)))
   {
     /*
       (the order of query cache and subst_spvars calls is irrelevant because
@@ -2619,8 +2630,10 @@ sp_instr_stmt::execute(THD *thd, uint *n
     }
     else
       *nextp= m_ip+1;
+    VOID(pthread_mutex_lock(&LOCK_thread_count));
     thd->query= query;
     thd->query_length= query_length;
+    VOID(pthread_mutex_unlock(&LOCK_thread_count));
   }
   DBUG_RETURN(res);
 }