#At file:///export/home/didrik/repo/next-mr-opt-backporting-wl4800/ based on revid:jorgen.loland@stripped
3309 Tor Didriksen 2011-05-19
WL#4800 Review comments.
modified:
WL4800_TODO.txt
sql/opt_trace.cc
sql/opt_trace.h
sql/opt_trace2server.cc
sql/sql_array.h
sql/sql_select.cc
=== modified file 'WL4800_TODO.txt'
--- a/WL4800_TODO.txt 2011-05-04 20:53:28 +0000
+++ b/WL4800_TODO.txt 2011-05-19 10:55:07 +0000
@@ -85,6 +85,8 @@ included everywhere. Of opt_trace.h, sql
Opt_trace_context. Should declaration of Opt_trace_context move to a
separate smaller include file, to be included in sql_class.h?
Guilhem suggests: yes.
+Tor: +1
C10) Guilhem to Tor: can we delete opt_notrace-t.cc now? I think I
replied in some old mail about this file.
+Tor: yes.
=== modified file 'sql/opt_trace.cc'
--- a/sql/opt_trace.cc 2011-05-05 15:47:46 +0000
+++ b/sql/opt_trace.cc 2011-05-19 10:55:07 +0000
@@ -211,6 +211,7 @@ public:
void assert_current_struct(const Opt_trace_struct *s) const
{ DBUG_ASSERT(current_struct == s); }
+ /// ?????????????????????????????????
void missing_privilege();
bool support_I_S() const { return I_S_disabled == 0; }
@@ -944,7 +945,7 @@ bool Opt_trace_context::start(bool suppo
/*
Decide on optimizations possible to realize the requested support.
- If I_S or debug output is requested, need to create an Opt_trace_stmt.
+ If I_S or debug output is requested, we need to create an Opt_trace_stmt.
Same if we should support calls to Opt_trace_context::missing_privilege(),
because that function requires an Opt_trace_stmt.
*/
=== modified file 'sql/opt_trace.h'
--- a/sql/opt_trace.h 2011-05-04 20:53:28 +0000
+++ b/sql/opt_trace.h 2011-05-19 10:55:07 +0000
@@ -25,6 +25,7 @@
struct st_schema_table;
struct TABLE_LIST;
struct TABLE;
+class sp_head;
class sp_instr;
/**
@@ -433,7 +434,7 @@ public:
member function has undefined effects.
*/
bool start(bool support_I_S,
- bool support_dbug_or_support_t_missing_priv,
+ bool support_dbug_or_support_missing_priv,
bool end_marker, bool one_line,
long offset, long limit, ulong max_mem_size,
ulonglong features);
@@ -684,6 +685,10 @@ private:
long since_offset_0;
};
+ // An XXX_impl class is usually hidden away in the .cc file
+ // but OK, I accept your request for inlining.
+
+ // The pointer to the impl is traditionally named 'pimpl'
Opt_trace_context_impl *impl;
/**
@@ -1308,12 +1313,14 @@ class st_select_lex;
void opt_trace_print_expanded_query(THD *thd,
st_select_lex *select_lex);
+// ???????????????????
void opt_trace_disable_if_no_view_access(THD *thd, TABLE_LIST *view,
- TABLE_LIST *underlying_tables);
+ TABLE_LIST *underlying_tables);
-class sp_head;
+// ???????????????????
void opt_trace_disable_if_no_stored_proc_func_access(THD *thd, sp_head *sp);
+// ???????????????????
void opt_trace_disable_if_no_security_context_access(THD *thd);
/**
@@ -1435,7 +1442,8 @@ public:
};
#define opt_trace_print_expanded_query(thd, select_lex) do {} while (0)
-#define opt_trace_disable_if_no_view_access(thd, view, underlying_tables) do {} while (0)
+#define opt_trace_disable_if_no_view_access(thd, view, underlying_tables) \
+ do {} while (0)
#define opt_trace_disable_if_no_stored_proc_func_access(thd, sp) do {} while (0)
#define opt_trace_disable_if_no_security_context_access(thd) do {} while (0)
=== modified file 'sql/opt_trace2server.cc'
--- a/sql/opt_trace2server.cc 2011-05-05 15:47:46 +0000
+++ b/sql/opt_trace2server.cc 2011-05-19 10:55:07 +0000
@@ -239,8 +239,8 @@ void opt_trace_print_expanded_query(THD
@name Description of trace-induced additional security checks.
A trace exposes information. For example if one does SELECT on a view, the
trace contains the view's body. So, the user should be allowed to see the
- trace only if she/he has privilege to see the body, i.e. privilege to do SHOW
- CREATE VIEW.
+ trace only if she/he has privilege to see the body, i.e. privilege to do
+ SHOW CREATE VIEW.
There are similar issues with stored procedures, functions, triggers.
We implement this by doing additional checks on SQL objects when tracing is
@@ -286,7 +286,7 @@ void opt_trace_print_expanded_query(THD
- from files (reading their content), with LOAD_FILE()
- from the list of connections (reading their queries...), with
I_S.PROCESSLIST.
- - from etc, maybe.
+ - from etc, maybe. ???
If the connected user has EXECUTE privilege on a routine which does a
security context change, the routine can retrieve information internally
(if allowed by the SUID context's privileges), and present only a portion
@@ -314,7 +314,7 @@ void opt_trace_disable_if_no_security_co
DBUG_ENTER("opt_trace_check_disable_if_no_security_context_access");
if (likely(!(thd->variables.optimizer_trace &
Opt_trace_context::FLAG_ENABLED)) || // (1)
- thd->system_thread) // (2)
+ thd->system_thread) // (2)
{
/*
(1) We know that the routine's execution starts with "enabled=off".
@@ -361,10 +361,13 @@ void opt_trace_disable_if_no_security_co
*/
if (!(test_all_bits(thd->main_security_ctx.master_access,
(GLOBAL_ACLS & ~GRANT_ACL))) &&
- (strcmp(thd->main_security_ctx.priv_user,
- thd->security_ctx->priv_user) ||
- my_strcasecmp(system_charset_info, thd->main_security_ctx.priv_host,
- thd->security_ctx->priv_host)))
+ ( 0 != strcmp(thd->main_security_ctx.priv_user,
+ thd->security_ctx->priv_user)
+ ||
+ 0 != my_strcasecmp(system_charset_info,
+ thd->main_security_ctx.priv_host,
+ thd->security_ctx->priv_host)
+ ))
trace->missing_privilege();
DBUG_VOID_RETURN;
#endif
@@ -449,12 +452,15 @@ void opt_trace_disable_if_no_view_access
Security_context * const backup_table_sctx= view->security_ctx;
Security_context * const backup_thd_sctx= thd->security_ctx;
const GRANT_INFO backup_grant_info= view->grant;
+
view->security_ctx= NULL; // no SUID context for view
thd->security_ctx= &thd->main_security_ctx; // no SUID context for THD
const int rc= check_table_access(thd, SHOW_VIEW_ACL, view, false, 1, true);
+
view->security_ctx= backup_table_sctx;
thd->security_ctx= backup_thd_sctx;
view->grant= backup_grant_info;
+
if (rc)
{
trace->missing_privilege();
@@ -583,10 +589,13 @@ int fill_optimizer_trace_info(THD *thd,
*/
if (!test_all_bits(thd->security_ctx->master_access,
(GLOBAL_ACLS & ~GRANT_ACL)) &&
- (strcmp(thd->main_security_ctx.priv_user,
- thd->security_ctx->priv_user) ||
- my_strcasecmp(system_charset_info, thd->main_security_ctx.priv_host,
- thd->security_ctx->priv_host)))
+ ( 0 != strcmp(thd->main_security_ctx.priv_user,
+ thd->security_ctx->priv_user)
+ ||
+ 0 != my_strcasecmp(system_charset_info,
+ thd->main_security_ctx.priv_host,
+ thd->security_ctx->priv_host)
+ ))
return 0;
/*
The list must not change during the iterator's life time. This is ok as
=== modified file 'sql/sql_array.h'
--- a/sql/sql_array.h 2011-04-05 07:34:13 +0000
+++ b/sql/sql_array.h 2011-05-19 10:55:07 +0000
@@ -73,6 +73,7 @@ public:
Pops the last element. The returned element must not be used after the
next update to the array.
*/
+ // This interface is still wrong!!
const Elem& pop()
{
return *static_cast<Elem *>(pop_dynamic(&array));
=== modified file 'sql/sql_select.cc'
--- a/sql/sql_select.cc 2011-05-13 14:02:31 +0000
+++ b/sql/sql_select.cc 2011-05-19 10:55:07 +0000
@@ -10269,6 +10269,7 @@ static bool make_join_select(JOIN *join,
sel->cond=orig_cond;
if (!*tab->on_expr_ref)
DBUG_RETURN(1); // Impossible WHERE
+// untabify above
Opt_trace_object trace_without_on(trace, "without_ON_clause");
if (sel->test_quick_select(thd, tab->keys,
used_tables & ~ current_map,
Attachment: [text/bzr-bundle] bzr/tor.didriksen@oracle.com-20110519105507-axasc7wx7m394lv3.bundle
