Skip to content

Commit eccd5ae

Browse files
author
kostja@vajra.(none)
committed
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.
1 parent 7d3c4c2 commit eccd5ae

8 files changed

Lines changed: 301 additions & 364 deletions

File tree

sql/event_data_objects.cc

Lines changed: 154 additions & 272 deletions
Large diffs are not rendered by default.

sql/event_data_objects.h

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717

1818

1919
#define EVEX_GET_FIELD_FAILED -2
20-
#define EVEX_COMPILE_ERROR -3
2120
#define EVEX_BAD_PARAMS -5
2221
#define EVEX_MICROSECOND_UNSUP -6
2322

@@ -169,28 +168,22 @@ class Event_timed : public Event_queue_element
169168
class Event_job_data : public Event_basic
170169
{
171170
public:
172-
sp_head *sphead;
173-
174171
LEX_STRING body;
175172
LEX_STRING definer_user;
176173
LEX_STRING definer_host;
177174

178175
ulong sql_mode;
179176

180177
Event_job_data();
181-
virtual ~Event_job_data();
182178

183179
virtual int
184180
load_from_row(THD *thd, TABLE *table);
185181

186-
int
182+
bool
187183
execute(THD *thd, bool drop);
188-
189-
int
190-
compile(THD *thd, MEM_ROOT *mem_root);
191184
private:
192-
int
193-
get_fake_create_event(String *buf);
185+
bool
186+
construct_sp_sql(THD *thd, String *sp_sql);
194187

195188
Event_job_data(const Event_job_data &); /* Prevent use of these */
196189
void operator=(Event_job_data &);

sql/event_scheduler.cc

Lines changed: 18 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -277,8 +277,7 @@ Event_worker_thread::run(THD *thd, Event_queue_element_for_exec *event)
277277
{
278278
/* needs to be first for thread_stack */
279279
char my_stack;
280-
int ret;
281-
Event_job_data *job_data= NULL;
280+
Event_job_data job_data;
282281
bool res;
283282

284283
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)
291290
if (res)
292291
goto end;
293292

294-
if (!(job_data= new Event_job_data()))
295-
goto end;
296-
else if ((ret= db_repository->
297-
load_named_event(thd, event->dbname, event->name, job_data)))
293+
if ((res= db_repository->load_named_event(thd, event->dbname, event->name,
294+
&job_data)))
298295
{
299-
DBUG_PRINT("error", ("Got %d from load_named_event", ret));
296+
DBUG_PRINT("error", ("Got error from load_named_event"));
300297
goto end;
301298
}
302299

303300
sql_print_information("Event Scheduler: "
304-
"[%s.%s of %s] executing in thread %lu. ",
305-
job_data->dbname.str, job_data->name.str,
306-
job_data->definer.str, thd->thread_id);
301+
"[%s].[%s.%s] started in thread %lu.",
302+
job_data.definer.str,
303+
job_data.dbname.str, job_data.name.str,
304+
thd->thread_id);
307305

308306
thd->enable_slow_log= TRUE;
309307

310-
ret= job_data->execute(thd, event->dropped);
308+
res= job_data.execute(thd, event->dropped);
311309

312-
print_warnings(thd, job_data);
310+
print_warnings(thd, &job_data);
313311

314-
switch (ret) {
315-
case 0:
312+
if (res)
313+
sql_print_information("Event Scheduler: "
314+
"[%s].[%s.%s] event execution failed.",
315+
job_data.definer.str,
316+
job_data.dbname.str, job_data.name.str);
317+
else
316318
sql_print_information("Event Scheduler: "
317319
"[%s].[%s.%s] executed successfully in thread %lu.",
318-
job_data->definer.str,
319-
job_data->dbname.str, job_data->name.str,
320+
job_data.definer.str,
321+
job_data.dbname.str, job_data.name.str,
320322
thd->thread_id);
321-
break;
322-
case EVEX_COMPILE_ERROR:
323-
sql_print_information("Event Scheduler: "
324-
"[%s].[%s.%s] event compilation failed.",
325-
job_data->definer.str,
326-
job_data->dbname.str, job_data->name.str);
327-
break;
328-
default:
329-
sql_print_information("Event Scheduler: "
330-
"[%s].[%s.%s] event execution failed.",
331-
job_data->definer.str,
332-
job_data->dbname.str, job_data->name.str);
333-
break;
334-
}
335323

336324
end:
337-
delete job_data;
338-
339325
DBUG_PRINT("info", ("Done with Event %s.%s", event->dbname.str,
340326
event->name.str));
341327

342328
delete event;
343329
deinit_event_thread(thd);
344-
/*
345-
Do not pthread_exit since we want local destructors for stack objects
346-
to be invoked.
347-
*/
348330
}
349331

350332

sql/item_func.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5351,7 +5351,7 @@ Item_func_sp::fix_fields(THD *thd, Item **ref)
53515351
Security_context *save_secutiry_ctx;
53525352
res= set_routine_security_ctx(thd, m_sp, false, &save_secutiry_ctx);
53535353
if (!res)
5354-
sp_restore_security_context(thd, save_secutiry_ctx);
5354+
m_sp->m_security_ctx.restore_security_context(thd, save_secutiry_ctx);
53555355

53565356
#endif /* ! NO_EMBEDDED_ACCESS_CHECKS */
53575357
}

sql/sp_head.cc

Lines changed: 8 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1232,7 +1232,11 @@ set_routine_security_ctx(THD *thd, sp_head *sp, bool is_proc,
12321232
Security_context **save_ctx)
12331233
{
12341234
*save_ctx= 0;
1235-
if (sp_change_security_context(thd, sp, save_ctx))
1235+
if (sp->m_chistics->suid != SP_IS_NOT_SUID &&
1236+
sp->m_security_ctx.change_security_context(thd, &sp->m_definer_user,
1237+
&sp->m_definer_host,
1238+
&sp->m_db,
1239+
save_ctx))
12361240
return TRUE;
12371241

12381242
/*
@@ -1249,7 +1253,7 @@ set_routine_security_ctx(THD *thd, sp_head *sp, bool is_proc,
12491253
check_routine_access(thd, EXECUTE_ACL,
12501254
sp->m_db.str, sp->m_name.str, is_proc, FALSE))
12511255
{
1252-
sp_restore_security_context(thd, *save_ctx);
1256+
sp->m_security_ctx.restore_security_context(thd, *save_ctx);
12531257
*save_ctx= 0;
12541258
return TRUE;
12551259
}
@@ -1560,7 +1564,7 @@ sp_head::execute_function(THD *thd, Item **argp, uint argcount,
15601564
}
15611565

15621566
#ifndef NO_EMBEDDED_ACCESS_CHECKS
1563-
sp_restore_security_context(thd, save_security_ctx);
1567+
m_security_ctx.restore_security_context(thd, save_security_ctx);
15641568
#endif
15651569

15661570
err_with_cleanup:
@@ -1778,7 +1782,7 @@ sp_head::execute_procedure(THD *thd, List<Item> *args)
17781782

17791783
#ifndef NO_EMBEDDED_ACCESS_CHECKS
17801784
if (save_security_ctx)
1781-
sp_restore_security_context(thd, save_security_ctx);
1785+
m_security_ctx.restore_security_context(thd, save_security_ctx);
17821786
#endif
17831787

17841788
if (!save_spcont)
@@ -3418,44 +3422,6 @@ sp_instr_set_case_expr::opt_move(uint dst, List<sp_instr> *bp)
34183422

34193423
/* ------------------------------------------------------------------ */
34203424

3421-
/*
3422-
Security context swapping
3423-
*/
3424-
3425-
#ifndef NO_EMBEDDED_ACCESS_CHECKS
3426-
bool
3427-
sp_change_security_context(THD *thd, sp_head *sp, Security_context **backup)
3428-
{
3429-
*backup= 0;
3430-
if (sp->m_chistics->suid != SP_IS_NOT_SUID &&
3431-
(strcmp(sp->m_definer_user.str,
3432-
thd->security_ctx->priv_user) ||
3433-
my_strcasecmp(system_charset_info, sp->m_definer_host.str,
3434-
thd->security_ctx->priv_host)))
3435-
{
3436-
if (acl_getroot_no_password(&sp->m_security_ctx, sp->m_definer_user.str,
3437-
sp->m_definer_host.str,
3438-
sp->m_definer_host.str,
3439-
sp->m_db.str))
3440-
{
3441-
my_error(ER_NO_SUCH_USER, MYF(0), sp->m_definer_user.str,
3442-
sp->m_definer_host.str);
3443-
return TRUE;
3444-
}
3445-
*backup= thd->security_ctx;
3446-
thd->security_ctx= &sp->m_security_ctx;
3447-
}
3448-
return FALSE;
3449-
}
3450-
3451-
void
3452-
sp_restore_security_context(THD *thd, Security_context *backup)
3453-
{
3454-
if (backup)
3455-
thd->security_ctx= backup;
3456-
}
3457-
3458-
#endif /* NO_EMBEDDED_ACCESS_CHECKS */
34593425

34603426
/*
34613427
Structure that represent all instances of one table

sql/sql_class.cc

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2119,6 +2119,102 @@ bool Security_context::set_user(char *user_arg)
21192119
return user == 0;
21202120
}
21212121

2122+
#ifndef NO_EMBEDDED_ACCESS_CHECKS
2123+
/**
2124+
Initialize this security context from the passed in credentials
2125+
and activate it in the current thread.
2126+
2127+
@param[out] backup Save a pointer to the current security context
2128+
in the thread. In case of success it points to the
2129+
saved old context, otherwise it points to NULL.
2130+
2131+
2132+
During execution of a statement, multiple security contexts may
2133+
be needed:
2134+
- the security context of the authenticated user, used as the
2135+
default security context for all top-level statements
2136+
- in case of a view or a stored program, possibly the security
2137+
context of the definer of the routine, if the object is
2138+
defined with SQL SECURITY DEFINER option.
2139+
2140+
The currently "active" security context is parameterized in THD
2141+
member security_ctx. By default, after a connection is
2142+
established, this member points at the "main" security context
2143+
- the credentials of the authenticated user.
2144+
2145+
Later, if we would like to execute some sub-statement or a part
2146+
of a statement under credentials of a different user, e.g.
2147+
definer of a procedure, we authenticate this user in a local
2148+
instance of Security_context by means of this method (and
2149+
ultimately by means of acl_getroot_no_password), and make the
2150+
local instance active in the thread by re-setting
2151+
thd->security_ctx pointer.
2152+
2153+
Note, that the life cycle and memory management of the "main" and
2154+
temporary security contexts are different.
2155+
For the main security context, the memory for user/host/ip is
2156+
allocated on system heap, and the THD class frees this memory in
2157+
its destructor. The only case when contents of the main security
2158+
context may change during its life time is when someone issued
2159+
CHANGE USER command.
2160+
Memory management of a "temporary" security context is
2161+
responsibility of the module that creates it.
2162+
2163+
@retval TRUE there is no user with the given credentials. The erro
2164+
is reported in the thread.
2165+
@retval FALSE success
2166+
*/
2167+
2168+
bool
2169+
Security_context::
2170+
change_security_context(THD *thd,
2171+
LEX_STRING *definer_user,
2172+
LEX_STRING *definer_host,
2173+
LEX_STRING *db,
2174+
Security_context **backup)
2175+
{
2176+
bool needs_change;
2177+
2178+
DBUG_ENTER("Security_context::change_security_context");
2179+
2180+
DBUG_ASSERT(definer_user->str && definer_host->str);
2181+
2182+
*backup= NULL;
2183+
/*
2184+
The current security context may have NULL members
2185+
if we have just started the thread and not authenticated
2186+
any user. This use case is currently in events worker thread.
2187+
*/
2188+
needs_change= (thd->security_ctx->priv_user == NULL ||
2189+
strcmp(definer_user->str, thd->security_ctx->priv_user) ||
2190+
thd->security_ctx->priv_host == NULL ||
2191+
my_strcasecmp(system_charset_info, definer_host->str,
2192+
thd->security_ctx->priv_host));
2193+
if (needs_change)
2194+
{
2195+
if (acl_getroot_no_password(this, definer_user->str, definer_host->str,
2196+
definer_host->str, db->str))
2197+
{
2198+
my_error(ER_NO_SUCH_USER, MYF(0), definer_user->str,
2199+
definer_host->str);
2200+
DBUG_RETURN(TRUE);
2201+
}
2202+
*backup= thd->security_ctx;
2203+
thd->security_ctx= this;
2204+
}
2205+
2206+
DBUG_RETURN(FALSE);
2207+
}
2208+
2209+
2210+
void
2211+
Security_context::restore_security_context(THD *thd,
2212+
Security_context *backup)
2213+
{
2214+
if (backup)
2215+
thd->security_ctx= backup;
2216+
}
2217+
#endif
21222218

21232219
/****************************************************************************
21242220
Handling of open and locked tables states.

sql/sql_class.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -656,6 +656,18 @@ class Security_context {
656656
}
657657

658658
bool set_user(char *user_arg);
659+
660+
#ifndef NO_EMBEDDED_ACCESS_CHECKS
661+
bool
662+
change_security_context(THD *thd,
663+
LEX_STRING *definer_user,
664+
LEX_STRING *definer_host,
665+
LEX_STRING *db,
666+
Security_context **backup);
667+
668+
void
669+
restore_security_context(THD *thd, Security_context *backup);
670+
#endif
659671
};
660672

661673

sql/sql_trigger.cc

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1543,9 +1543,15 @@ bool Table_triggers_list::process_triggers(THD *thd, trg_event_type event,
15431543
old_field= trigger_table->field;
15441544
}
15451545
#ifndef NO_EMBEDDED_ACCESS_CHECKS
1546+
Security_context *sctx= &sp_trigger->m_security_ctx;
15461547
Security_context *save_ctx;
15471548

1548-
if (sp_change_security_context(thd, sp_trigger, &save_ctx))
1549+
1550+
if (sctx->change_security_context(thd,
1551+
&sp_trigger->m_definer_user,
1552+
&sp_trigger->m_definer_host,
1553+
&sp_trigger->m_db,
1554+
&save_ctx))
15491555
return TRUE;
15501556

15511557
/*
@@ -1570,7 +1576,7 @@ bool Table_triggers_list::process_triggers(THD *thd, trg_event_type event,
15701576
thd->security_ctx->priv_user, thd->security_ctx->host_or_ip,
15711577
trigger_table->s->table_name.str);
15721578

1573-
sp_restore_security_context(thd, save_ctx);
1579+
sctx->restore_security_context(thd, save_ctx);
15741580
return TRUE;
15751581
}
15761582
#endif // NO_EMBEDDED_ACCESS_CHECKS
@@ -1582,7 +1588,7 @@ bool Table_triggers_list::process_triggers(THD *thd, trg_event_type event,
15821588
thd->restore_sub_statement_state(&statement_state);
15831589

15841590
#ifndef NO_EMBEDDED_ACCESS_CHECKS
1585-
sp_restore_security_context(thd, save_ctx);
1591+
sctx->restore_security_context(thd, save_ctx);
15861592
#endif // NO_EMBEDDED_ACCESS_CHECKS
15871593
}
15881594

0 commit comments

Comments
 (0)