Post review and additional fix for BUG#10968: Stored procedures: crash if long loop.

  Fixed valgrind complaints. This fixes the memory leak problems for
  procedured, and partially for functions. There's still a leak involving
  results from functions that turned out to be too involved, so it will be
  fixed separately.


mysql-test/r/sp.result:
  Fixed some minor mistake (spotted while debugging).
mysql-test/t/sp.test:
  Fixed some minor mistake (spotted while debugging).
sql/item_func.cc:
  Moved Item_func_sp::cleanup() from item_func.h to ease debugging,
  and made a debug output come out right.
sql/item_func.h:
  Moved Item_func_sp::cleanup() to item_func.cc to ease debugging.
sql/sp_head.cc:
  Fixed valgrind problems with the previous memory leak fix (unit cleanup and
  putting result field in a differen mem_root), and removed prealloc flag from
  init_alloc_root() calls.
sql/sp_rcontext.cc:
  New mem_root pointer used for return fields from functions.
sql/sp_rcontext.h:
  New mem_root pointer used for return fields from functions.

7 files changed, 43 insertions, 19 deletions

diff --git a/mysql-test/r/sp.result b/mysql-test/r/sp.result
index 3c6fa84882f..1741070669f 100644
--- a/mysql-test/r/sp.result
+++ b/mysql-test/r/sp.result
@@ -251,7 +251,7 @@ call sub1("sub1a", (select 7))|
 call sub1("sub1b", (select max(i) from t2))|
 call sub1("sub1c", (select i,d from t2 limit 1))|
 call sub1("sub1d", (select 1 from (select 1) a))|
-call sub2("sub2");
+call sub2("sub2")|
 select * from t1|
 id	data
 sub1a	7
@@ -265,6 +265,7 @@ sub3((select max(i) from t2))
 drop procedure sub1|
 drop procedure sub2|
 drop function sub3|
+delete from t1|
 delete from t2|
 drop procedure if exists a0|
 create procedure a0(x int)
@@ -275,11 +276,6 @@ end while|
 call a0(3)|
 select * from t1|
 id	data
-sub1a	7
-sub1b	3
-sub1c	1
-sub1d	1
-sub2	6
 a0	2
 a0	1
 a0	0
diff --git a/mysql-test/t/sp.test b/mysql-test/t/sp.test
index 7acd4d81081..e76dc971df7 100644
--- a/mysql-test/t/sp.test
+++ b/mysql-test/t/sp.test
@@ -372,12 +372,13 @@ call sub1("sub1a", (select 7))|
 call sub1("sub1b", (select max(i) from t2))|
 call sub1("sub1c", (select i,d from t2 limit 1))|
 call sub1("sub1d", (select 1 from (select 1) a))|
-call sub2("sub2");
+call sub2("sub2")|
 select * from t1|
 select sub3((select max(i) from t2))|
 drop procedure sub1|
 drop procedure sub2|
 drop function sub3|
+delete from t1|
 delete from t2|
 
 # Basic tests of the flow control constructs
diff --git a/sql/item_func.cc b/sql/item_func.cc
index 5af99cb8132..4a12c6529e6 100644
--- a/sql/item_func.cc
+++ b/sql/item_func.cc
@@ -4698,6 +4698,16 @@ Item_func_sp::Item_func_sp(sp_name *name, List<Item> &list)
   dummy_table= (TABLE*) sql_calloc(sizeof(TABLE));
 }
 
+void
+Item_func_sp::cleanup()
+{
+  if (result_field)
+  {
+    delete result_field;
+    result_field= NULL;
+  }
+  Item_func::cleanup();
+}
 
 const char *
 Item_func_sp::func_name() const
@@ -4746,7 +4756,8 @@ Item_func_sp::sp_result_field(void) const
     share->table_cache_key = empty_name;
     share->table_name = empty_name;
   }
-  DBUG_RETURN(m_sp->make_field(max_length, name, dummy_table));
+  field= m_sp->make_field(max_length, name, dummy_table);
+  DBUG_RETURN(field);
 }
 
 
diff --git a/sql/item_func.h b/sql/item_func.h
index f0c7e25ad53..1ac1449760f 100644
--- a/sql/item_func.h
+++ b/sql/item_func.h
@@ -1308,13 +1308,7 @@ public:
   virtual ~Item_func_sp()
   {}
 
-  void cleanup()
-  {
-    if (result_field)
-      delete result_field;
-    Item_func::cleanup();
-    result_field= NULL;
-  }
+  void cleanup();
 
   const char *func_name() const;
 
@@ -1330,7 +1324,7 @@ public:
   {
     if (execute(&result_field))
       return (longlong) 0;
-    return result_field->val_int();   
+    return result_field->val_int();
   }
 
   double val_real()
diff --git a/sql/sp_head.cc b/sql/sp_head.cc
index bd051cb7480..fd5ad67e612 100644
--- a/sql/sp_head.cc
+++ b/sql/sp_head.cc
@@ -534,17 +534,35 @@ sp_head::destroy()
 }
 
 
+/*
+ *  This is only used for result fields from functions (both during
+ *  fix_length_and_dec() and evaluation).
+ *
+ *  Since the current mem_root during a will be freed and the result
+ *  field will be used by the caller, we have to put it in the caller's
+ *  or main mem_root.
+ */
 Field *
 sp_head::make_field(uint max_length, const char *name, TABLE *dummy)
 {
   Field *field;
+  MEM_ROOT *tmp_mem_root;
+  THD *thd;
   DBUG_ENTER("sp_head::make_field");
+
+  thd= current_thd;
+  tmp_mem_root= thd->mem_root;
+  if (thd->spcont && thd->spcont->callers_mem_root)
+    thd->mem_root= thd->spcont->callers_mem_root;
+  else
+    thd->mem_root= &thd->main_mem_root;
   field= ::make_field((char *)0,
 		!m_returns_len ? max_length : m_returns_len, 
 		(uchar *)"", 0, m_returns_pack, m_returns, m_returns_cs,
 		(enum Field::geometry_type)0, Field::NONE, 
 		m_returns_typelib,
 		name ? name : (const char *)m_name.str, dummy);
+  thd->mem_root= tmp_mem_root;
   DBUG_RETURN(field);
 }
 
@@ -708,7 +726,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount, Item **resp)
     DBUG_RETURN(-1);
   }
 
-  init_alloc_root(&call_mem_root, MEM_ROOT_BLOCK_SIZE, MEM_ROOT_PREALLOC);
+  init_alloc_root(&call_mem_root, MEM_ROOT_BLOCK_SIZE, 0);
   old_mem_root= thd->mem_root;
   thd->mem_root= &call_mem_root;
   old_free_list= thd->free_list; // Keep the old list
@@ -716,7 +734,8 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount, Item **resp)
 
   // QQ Should have some error checking here? (types, etc...)
   nctx= new sp_rcontext(csize, hmax, cmax);
-  for (i= 0 ; i < params && i < argcount ; i++)
+  nctx->callers_mem_root= old_mem_root;
+  for (i= 0 ; i < argcount ; i++)
   {
     sp_pvar_t *pvar = m_pcont->find_pvar(i);
     Item *it= sp_eval_func_item(thd, argp++, pvar->type, NULL);
@@ -812,7 +831,7 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
     DBUG_RETURN(-1);
   }
 
-  init_alloc_root(&call_mem_root, MEM_ROOT_BLOCK_SIZE, MEM_ROOT_PREALLOC);
+  init_alloc_root(&call_mem_root, MEM_ROOT_BLOCK_SIZE, 0);
   old_mem_root= thd->mem_root;
   thd->mem_root= &call_mem_root;
   old_free_list= thd->free_list; // Keep the old list
@@ -965,6 +984,7 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
   // Now get rid of the rest of the callee context
   cleanup_items(call_free_list);
   free_items(call_free_list);
+  thd->lex->unit.cleanup();
   free_root(&call_mem_root, MYF(0));
 
   DBUG_RETURN(ret);
diff --git a/sql/sp_rcontext.cc b/sql/sp_rcontext.cc
index 2e79a1e2533..daca6b38b46 100644
--- a/sql/sp_rcontext.cc
+++ b/sql/sp_rcontext.cc
@@ -32,6 +32,7 @@ sp_rcontext::sp_rcontext(uint fsize, uint hmax, uint cmax)
   : m_count(0), m_fsize(fsize), m_result(NULL), m_hcount(0), m_hsp(0),
     m_hfound(-1), m_ccount(0)
 {
+  callers_mem_root= NULL;
   in_handler= FALSE;
   m_frame= (Item **)sql_alloc(fsize * sizeof(Item*));
   m_handler= (sp_handler_t *)sql_alloc(hmax * sizeof(sp_handler_t));
diff --git a/sql/sp_rcontext.h b/sql/sp_rcontext.h
index ba5fa950dc3..864dc3df146 100644
--- a/sql/sp_rcontext.h
+++ b/sql/sp_rcontext.h
@@ -47,6 +47,7 @@ class sp_rcontext : public Sql_alloc
 
  public:
 
+  MEM_ROOT *callers_mem_root;	// Used to store result fields
   bool in_handler;
 
   sp_rcontext(uint fsize, uint hmax, uint cmax);