* Gleb Shchepa <gshchepa@stripped> [08/11/12 13:51]:
> 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.
Since when did we agree to protect the assignment of
thd->query/query_length with LOCK_thread_count?
If we decided to do that, why don't you change alloc_query() and
other places as well?
Serg, I support this approach, do you?
>
> 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);
> }
--
