An attempt to fix a sporadic valgrind memory leak in Event Scheduler:

streamline the event worker thread work flow and try to eliminate
possibilities for memory corruptions that might have been
lurking in previous (complicated) code.
This patch: 
 * removes Event_job_data::compile that was never used
 * cleans up Event_job_data::execute to minimize juggling with
   thread context and eliminate unneded code paths
 * Implements Security_context::change/restore_security_context
   to be able to re-use these methods in all stored programs
This is to maybe fix Bug#27733 "Valgrind failures in 
remove_table_from_cache".
Review comments applied.


sql/event_data_objects.cc:
  Remove Event_job_data::compile, which was never used without
  Event_job_data::execute().
  Merge the implementation of compile() with Event_job_data::execute().
  Reuse existing functions to prepare the event worker thread
  for execution instead of some previously copy-pasted code.
  Do not change and restore the current database inside 
  Event_job_data::execute(), just set the current database in the 
  thread, that is enough to parse and execute an event.
sql/event_data_objects.h:
  Update declarations.
sql/event_scheduler.cc:
  Allocate Event_job_data on stack.
sql/item_func.cc:
  Update to match the new declaration of restore_security_context()
sql/sp_head.cc:
  Update to match the new declaration of 
  change/restore_security_context()
sql/sql_class.cc:
  Move change/restore_security_context to class Security_context.
  Add more comments.
sql/sql_class.h:
  Make change/restore_security_context methods of Security_context.
  That allows us to reuse them in Event Scheduler (instead of a
  copy-paste presently used there).
sql/sql_trigger.cc:
  Update to match the new declaration of 
  change/restore_security_context()

sql/event_data_objects.h

@@ -17,7 +17,6 @@
 
 
 #define EVEX_GET_FIELD_FAILED   -2
-#define EVEX_COMPILE_ERROR      -3
 #define EVEX_BAD_PARAMS         -5
 #define EVEX_MICROSECOND_UNSUP  -6
 
@@ -169,8 +168,6 @@ class Event_timed : public Event_queue_element
 class Event_job_data : public Event_basic
 {
 public:
-  sp_head *sphead;
-
   LEX_STRING body;
   LEX_STRING definer_user;
   LEX_STRING definer_host;
@@ -178,19 +175,15 @@ class Event_job_data : public Event_basic
   ulong sql_mode;
 
   Event_job_data();
-  virtual ~Event_job_data();
 
   virtual int
   load_from_row(THD *thd, TABLE *table);
 
-  int
+  bool
   execute(THD *thd, bool drop);
-
-  int
-  compile(THD *thd, MEM_ROOT *mem_root);
 private:
-  int
-  get_fake_create_event(String *buf);
+  bool
+  construct_sp_sql(THD *thd, String *sp_sql);
 
   Event_job_data(const Event_job_data &);       /* Prevent use of these */
   void operator=(Event_job_data &);

sql/event_scheduler.cc

@@ -277,8 +277,7 @@ Event_worker_thread::run(THD *thd, Event_queue_element_for_exec *event)
 {
   /* needs to be first for thread_stack */
   char my_stack;
-  int ret;
-  Event_job_data *job_data= NULL;
+  Event_job_data job_data;
   bool res;
 
   thd->thread_stack= &my_stack;                // remember where our stack is
@@ -291,60 +290,43 @@ Event_worker_thread::run(THD *thd, Event_queue_element_for_exec *event)
   if (res)
     goto end;
 
-  if (!(job_data= new Event_job_data()))
-    goto end;
-  else if ((ret= db_repository->
-                  load_named_event(thd, event->dbname, event->name, job_data)))
+  if ((res= db_repository->load_named_event(thd, event->dbname, event->name,
+                                            &job_data)))
   {
-    DBUG_PRINT("error", ("Got %d from load_named_event", ret));
+    DBUG_PRINT("error", ("Got error from load_named_event"));
     goto end;
   }
 
   sql_print_information("Event Scheduler: "
-                        "[%s.%s of %s] executing in thread %lu. ",
-                        job_data->dbname.str, job_data->name.str,
-                        job_data->definer.str, thd->thread_id);
+                        "[%s].[%s.%s] started in thread %lu.",
+                        job_data.definer.str,
+                        job_data.dbname.str, job_data.name.str,
+                        thd->thread_id);
 
   thd->enable_slow_log= TRUE;
 
-  ret= job_data->execute(thd, event->dropped);
+  res= job_data.execute(thd, event->dropped);
 
-  print_warnings(thd, job_data);
+  print_warnings(thd, &job_data);
 
-  switch (ret) {
-  case 0:
+  if (res)
+    sql_print_information("Event Scheduler: "
+                          "[%s].[%s.%s] event execution failed.",
+                          job_data.definer.str,
+                          job_data.dbname.str, job_data.name.str);
+  else
     sql_print_information("Event Scheduler: "
                           "[%s].[%s.%s] executed successfully in thread %lu.",
-                          job_data->definer.str,
-                          job_data->dbname.str, job_data->name.str,
+                          job_data.definer.str,
+                          job_data.dbname.str, job_data.name.str,
                           thd->thread_id);
-    break;
-  case EVEX_COMPILE_ERROR:
-    sql_print_information("Event Scheduler: "
-                          "[%s].[%s.%s] event compilation failed.",
-                          job_data->definer.str,
-                          job_data->dbname.str, job_data->name.str);
-    break;
-  default:
-    sql_print_information("Event Scheduler: "
-                          "[%s].[%s.%s] event execution failed.",
-                          job_data->definer.str,
-                          job_data->dbname.str, job_data->name.str);
-    break;
-  }
 
 end:
-  delete job_data;
-
   DBUG_PRINT("info", ("Done with Event %s.%s", event->dbname.str,
              event->name.str));
 
   delete event;
   deinit_event_thread(thd);
-  /*
-    Do not pthread_exit since we want local destructors for stack objects
-    to be invoked.
-  */
 }
 
 

sql/item_func.cc

@@ -5351,7 +5351,7 @@ Item_func_sp::fix_fields(THD *thd, Item **ref)
     Security_context *save_secutiry_ctx;
     res= set_routine_security_ctx(thd, m_sp, false, &save_secutiry_ctx);
     if (!res)
-      sp_restore_security_context(thd, save_secutiry_ctx);
+      m_sp->m_security_ctx.restore_security_context(thd, save_secutiry_ctx);
     
 #endif /* ! NO_EMBEDDED_ACCESS_CHECKS */
   }

sql/sp_head.cc

@@ -1232,7 +1232,11 @@ set_routine_security_ctx(THD *thd, sp_head *sp, bool is_proc,
                          Security_context **save_ctx)
 {
   *save_ctx= 0;
-  if (sp_change_security_context(thd, sp, save_ctx))
+  if (sp->m_chistics->suid != SP_IS_NOT_SUID &&
+      sp->m_security_ctx.change_security_context(thd, &sp->m_definer_user,
+                                                 &sp->m_definer_host,
+                                                 &sp->m_db,
+                                                 save_ctx))
     return TRUE;
 
   /*
@@ -1249,7 +1253,7 @@ set_routine_security_ctx(THD *thd, sp_head *sp, bool is_proc,
       check_routine_access(thd, EXECUTE_ACL,
                            sp->m_db.str, sp->m_name.str, is_proc, FALSE))
   {
-    sp_restore_security_context(thd, *save_ctx);
+    sp->m_security_ctx.restore_security_context(thd, *save_ctx);
     *save_ctx= 0;
     return TRUE;
   }
@@ -1560,7 +1564,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount,
   }
 
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
-  sp_restore_security_context(thd, save_security_ctx);
+  m_security_ctx.restore_security_context(thd, save_security_ctx);
 #endif
 
 err_with_cleanup:
@@ -1778,7 +1782,7 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
 
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
   if (save_security_ctx)
-    sp_restore_security_context(thd, save_security_ctx);
+    m_security_ctx.restore_security_context(thd, save_security_ctx);
 #endif
 
   if (!save_spcont)
@@ -3418,44 +3422,6 @@ sp_instr_set_case_expr::opt_move(uint dst, List<sp_instr> *bp)
 
 /* ------------------------------------------------------------------ */
 
-/*
-  Security context swapping
-*/
-
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-bool
-sp_change_security_context(THD *thd, sp_head *sp, Security_context **backup)
-{
-  *backup= 0;
-  if (sp->m_chistics->suid != SP_IS_NOT_SUID &&
-      (strcmp(sp->m_definer_user.str,
-              thd->security_ctx->priv_user) ||
-       my_strcasecmp(system_charset_info, sp->m_definer_host.str,
-                     thd->security_ctx->priv_host)))
-  {
-    if (acl_getroot_no_password(&sp->m_security_ctx, sp->m_definer_user.str,
-                                sp->m_definer_host.str,
-                                sp->m_definer_host.str,
-                                sp->m_db.str))
-    {
-      my_error(ER_NO_SUCH_USER, MYF(0), sp->m_definer_user.str,
-               sp->m_definer_host.str);
-      return TRUE;
-    }
-    *backup= thd->security_ctx;
-    thd->security_ctx= &sp->m_security_ctx;
-  }
-  return FALSE;
-}
-
-void
-sp_restore_security_context(THD *thd, Security_context *backup)
-{
-  if (backup)
-    thd->security_ctx= backup;
-}
-
-#endif /* NO_EMBEDDED_ACCESS_CHECKS */
 
 /*
   Structure that represent all instances of one table

sql/sql_class.cc

@@ -2119,6 +2119,102 @@ bool Security_context::set_user(char *user_arg)
   return user == 0;
 }
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+/**
+  Initialize this security context from the passed in credentials
+  and activate it in the current thread.
+
+  @param[out]  backup  Save a pointer to the current security context
+                       in the thread. In case of success it points to the
+                       saved old context, otherwise it points to NULL.
+
+
+  During execution of a statement, multiple security contexts may
+  be needed:
+  - the security context of the authenticated user, used as the
+    default security context for all top-level statements
+  - in case of a view or a stored program, possibly the security
+    context of the definer of the routine, if the object is
+    defined with SQL SECURITY DEFINER option.
+
+  The currently "active" security context is parameterized in THD
+  member security_ctx. By default, after a connection is
+  established, this member points at the "main" security context
+  - the credentials of the authenticated user.
+
+  Later, if we would like to execute some sub-statement or a part
+  of a statement under credentials of a different user, e.g.
+  definer of a procedure, we authenticate this user in a local
+  instance of Security_context by means of this method (and
+  ultimately by means of acl_getroot_no_password), and make the
+  local instance active in the thread by re-setting
+  thd->security_ctx pointer.
+
+  Note, that the life cycle and memory management of the "main" and
+  temporary security contexts are different.
+  For the main security context, the memory for user/host/ip is
+  allocated on system heap, and the THD class frees this memory in
+  its destructor. The only case when contents of the main security
+  context may change during its life time is when someone issued
+  CHANGE USER command.
+  Memory management of a "temporary" security context is
+  responsibility of the module that creates it.
+
+  @retval TRUE  there is no user with the given credentials. The erro
+                is reported in the thread.
+  @retval FALSE success
+*/
+
+bool
+Security_context::
+change_security_context(THD *thd,
+                        LEX_STRING *definer_user,
+                        LEX_STRING *definer_host,
+                        LEX_STRING *db,
+                        Security_context **backup)
+{
+  bool needs_change;
+
+  DBUG_ENTER("Security_context::change_security_context");
+
+  DBUG_ASSERT(definer_user->str && definer_host->str);
+
+  *backup= NULL;
+  /*
+    The current security context may have NULL members
+    if we have just started the thread and not authenticated
+    any user. This use case is currently in events worker thread.
+  */
+  needs_change= (thd->security_ctx->priv_user == NULL ||
+                 strcmp(definer_user->str, thd->security_ctx->priv_user) ||
+                 thd->security_ctx->priv_host == NULL ||
+                 my_strcasecmp(system_charset_info, definer_host->str,
+                               thd->security_ctx->priv_host));
+  if (needs_change)
+  {
+    if (acl_getroot_no_password(this, definer_user->str, definer_host->str,
+                                definer_host->str, db->str))
+    {
+      my_error(ER_NO_SUCH_USER, MYF(0), definer_user->str,
+               definer_host->str);
+      DBUG_RETURN(TRUE);
+    }
+    *backup= thd->security_ctx;
+    thd->security_ctx= this;
+  }
+
+  DBUG_RETURN(FALSE);
+}
+
+
+void
+Security_context::restore_security_context(THD *thd,
+                                           Security_context *backup)
+{
+  if (backup)
+    thd->security_ctx= backup;
+}
+#endif
 
 /****************************************************************************
   Handling of open and locked tables states.

sql/sql_class.h

@@ -656,6 +656,18 @@ class Security_context {
   }
   
   bool set_user(char *user_arg);
+
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  bool
+  change_security_context(THD *thd,
+                          LEX_STRING *definer_user,
+                          LEX_STRING *definer_host,
+                          LEX_STRING *db,
+                          Security_context **backup);
+
+  void
+  restore_security_context(THD *thd, Security_context *backup);
+#endif
 };
 
 

sql/sql_trigger.cc

@@ -1543,9 +1543,15 @@ bool Table_triggers_list::process_triggers(THD *thd, trg_event_type event,
       old_field= trigger_table->field;
     }
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
+    Security_context *sctx= &sp_trigger->m_security_ctx;
     Security_context *save_ctx;
 
-    if (sp_change_security_context(thd, sp_trigger, &save_ctx))
+
+    if (sctx->change_security_context(thd,
+                                      &sp_trigger->m_definer_user,
+                                      &sp_trigger->m_definer_host,
+                                      &sp_trigger->m_db,
+                                      &save_ctx))
       return TRUE;
 
     /*
@@ -1570,7 +1576,7 @@ bool Table_triggers_list::process_triggers(THD *thd, trg_event_type event,
                thd->security_ctx->priv_user, thd->security_ctx->host_or_ip,
                trigger_table->s->table_name.str);
 
-      sp_restore_security_context(thd, save_ctx);
+      sctx->restore_security_context(thd, save_ctx);
       return TRUE;
     }
 #endif // NO_EMBEDDED_ACCESS_CHECKS
@@ -1582,7 +1588,7 @@ bool Table_triggers_list::process_triggers(THD *thd, trg_event_type event,
     thd->restore_sub_statement_state(&statement_state);
 
 #ifndef NO_EMBEDDED_ACCESS_CHECKS
-    sp_restore_security_context(thd, save_ctx);
+    sctx->restore_security_context(thd, save_ctx);
 #endif // NO_EMBEDDED_ACCESS_CHECKS
   }