Hello,
Tor Didriksen a écrit, Le 19.05.2011 12:55:
> #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 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
Ok, I have done it as a revision consisting of only a pure cut-and-paste
so that you don't have to review it thoroughly.
> 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.
Done.
What about the other points in the todo file?
You can ignore point C6 I just implemented it (Jorgen was ok and I
thought you would also be ;-)
> === 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();
Added comment.
> 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.
done
> 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;
done
> 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,
done
> 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.
thanks!
> +
> + // The pointer to the impl is traditionally named 'pimpl'
renamed to 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,
This and the other similar functions have their doxygen comments in
opt_trace2server.cc; would you like that I move them here?
Then I'd also move the one which starts with
/**
@name Description of trace-induced additional security checks.
> - 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)
done
> #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.
done
> 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. ???
That meant that I'm not sure what else. But neither Dmitry nor me had an
idea of what this could be.
I deleted this line.
> 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)
done
> {
> /*
> (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)
> + ))
done
> 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;
> +
all done
> 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)
> + ))
done
> 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!!
Sure... but in what sense?
> 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
done
> Opt_trace_object trace_without_on(trace, "without_ON_clause");
> if (sel->test_quick_select(thd, tab->keys,
> used_tables & ~ current_map,
>
Changes are pushed.
