List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:May 20 2011 1:17pm
Subject:Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800
View as plain text  
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.
Thread
bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Tor Didriksen19 May
  • Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Jorgen Loland19 May
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Jorgen Loland19 May
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Guilhem Bichot20 May
      • Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Jorgen Loland20 May
  • Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Guilhem Bichot20 May
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Tor Didriksen20 May
      • Re: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800Guilhem Bichot21 May