From: Tor Didriksen Date: May 19 2011 10:55am Subject: bzr commit into mysql-trunk branch (tor.didriksen:3309) WL#4800 List-Archive: http://lists.mysql.com/commits/137687 Message-Id: <20110519105512.9F1EE37E1@atum07.norway.sun.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8931639458740056535==" --===============8931639458740056535== MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline #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(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, --===============8931639458740056535== MIME-Version: 1.0 Content-Type: text/bzr-bundle; charset="us-ascii"; name="bzr/tor.didriksen@stripped" Content-Transfer-Encoding: 7bit Content-Disposition: inline # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: tor.didriksen@stripped\ # axasc7wx7m394lv3 # target_branch: file:///export/home/didrik/repo/next-mr-opt-\ # backporting-wl4800/ # testament_sha1: 7afdddc173e1b7c352264659fdca0957b029f9d5 # timestamp: 2011-05-19 12:55:12 +0200 # base_revision_id: jorgen.loland@stripped\ # 2df6qg7h9ymrj17c # # Begin bundle IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWcWGvysABbB/gFwwwIJ6//// /mydxL////5gCk+PDMDQAAAW1KAUAAAAEok0U8mp6ajNU9T8phoUz09UyTGgE8psgIwgbRNBxkya NAaNMRkaGIYE0aYgxGgwgAMOMmTRoDRpiMjQxDAmjTEGI0GEABg01GginlNNMEyAMhoAAADTQ0DE GQOMmTRoDRpiMjQxDAmjTEGI0GEABgkkE0ABMSZMTSZkmJqaZU/U0aepqPKYmg0DT2qaeMEoScwW 3r4tWmXcU2pqzXNL0yMtzxhrxeOE9FNl/SzE/Ias+x+Th94mKp8/LGJBgsvHI98WmjS6JEAGZBhM KEyt85RIRfSyaeUWgNvwoaeUdFvSZkCTxKxkOY82cMi1ypBv0wms+vwmiyOovM/QVImZhCSSQgJQ Ej3vIEFKn8vnILVqVQ5BWarWuiGo8gtraqhjGOrtcriXNEVMGCUywDIqx8QXtEjAxpkLypSJM2VT QeGHQvZcJIxHWufi7rCmF/XtUDgzoBKGCbpVlIsssl+ounbDsFkJF6el+VwRbcilUSjYaJRM4BKk kKDCL4RS0MYf6N1mPhsVgw4nVYHNatV/hv/tRZcaG7HEJJRjepgpEHlBE8d+ahegVPQvKV7WQsGA sYYc2dkZ7axgIjzydvGso+BaESfiMOHDHsgXyXEpNqKg/UGREAo4e7u8Ck5+sugSHYGGs5naisOp u+dLuhzy1FuhNYw4cMITIDq6xMZLUP68T2EU/p5J705kp+1FFLxPv3e4kCN7HRgRsYJpR/ZCHBpS JnDC3zjiZQZgJh45ITHEgnBFhh8X1FZOPRGI6IyQOFOOAJPgwwg5EziiBEcTkUD5KK4d59/3SH2K DA8SCMhtCuKopJHKvcqoiKUE0pbTUNhTbCdQaMbRGEJA8qcwCiAXGXGEyBTZqeaVb2jMaE6e+LeO GcqmoxGwmvqHszIY3YWTudqBtJf7sML2ylSTlYxNgQP7LEjt/CYJlOLU8NsACrM0IGQtSgxR2Mi4 8z7XGFNuiqfDe7upvUWQBqXE1CmneaDz6EiyVgmUh46ewgsuZA5TEzTqCpzGCiNYxYVmsUFISL01 WCLlA+JJ1VrFkHcbH5FU76BMzMKiom4kYH3YlUJpOFWNeMRKyLx4zrTFXkpFBORIFDyDng6GR6AF BiUIdwLXmNZUYFZWjJFsL5pSwKWgzZXTxjQ9nLNktU+mWCcKcnHGWzhOU0DIHLUudS1Q1Q0Lx1lk isnuI/0+neoF1pcUJVETM1GLIGBmfY+yO8pMAB9Qew4np0S0c1zgsiDkA5y1mxNCERjIZILGE9Ow eVGCKimhjtsNjyQdlABOUkWWiocXQgJibswfcU9Y5mMay4wKC6BoSNvQPiQnnxzzZ8M6SMSJEex3 hIwrLyVxtLyVBDPaaui0bsyx5M5jbTSmSYOZI+cMz3m0+ZsWKNDEuMNrGmTtszYzaxWaAlOYQpam l3LsRvP6SqrTicrB1Z7x1Txw0xRcLdcBSPIuBiomH0BllsTzmAUh2RNwBsKpU3YtpdF8bWQ8gi/E nYiptB9JrSbju7moFQTqsq0KCmRXJQJy0t2S12FmJFSKjF5YRKpjGuAzTuNg6xATmFRgSOOJEAl6 SJ8iUSkgWUyDEpHkjIxKjIp3LmeR0LNJOVdbqWC/SsTpMnMlAcA0zQSnQTSaX4W/FLT8Qq+miHDD JpmZmQMfeJxw+YvL4IX5iKmBgpFBIoALlx2SBMQUBKHCq9Bf+Qu8V1+JrN+TtFv9xTwxnGr6ySar D75xTDK8zbjfcVsdXLvpSZgL1vr0gxHhFKVLHjkDfh+J8vnIK1MClUfKcsuJkiwP3/eViX3FwziF hlOPQc5mITKdesRDSX4qXc4L8n7QJckYIJQi4H4mRXBBtD6/48QL7puK7Mb0linr8gDaTFhi1P7A DvrUPjIyM+JcXnduORvJjcjHmcCRsMqg7533NoEfCW1qes7scUGQANgLqiWjHDMU/UkNA4NoYOOg BqTHcY6iJQNgwDmUzsHni95b03dBwpM23a4Qn+4rLi0QpaNwIqkDMckU9HMgzR3vC3TqcyzDsOBQ ZgTa0gdQhwVR2GFaFFTlww2XCNZp+RwPz5V1lWXiu1AeKV0tC0t/j6mCQMe/qcurOExt9Gyfl5lu /KSoEJlJwu03Eo8ihy2jROlvY6yMOJvPCY3DTeonCyuM2Q6OKV6B3XYN5+Nuw6FCYCyiqzkSJzWm jr17Ziepj80VkPMuJOOse1rv+HFqzFinVaJyA0S5rRwHHJxPMEyn3QIPOrlkenjZVZ6s0k7RKgQp PXQSHK2e6ht/MexTOGj49UUW0NsIKxAjICJJVVHj4uMWI2B3HRKdLghjX9RdROFZuKylGJ3blJXt mOr8eEy0PJPerQ1EWp+SOyoWqPcW+Yuc55G9P1EmWzmC/Nv3x+63yFL4TRCgVRA7adpvMzikKCKm PY3cjcbhaU8cEL0+vRAQr4K2UUrSr3nutEE5jYwgcd4q1VX4se4OxBPpMS2cfU405N/LrzqnQU3G M4a5INHNmA8ZInFa5bQoMixaHdtG6laJHgbUHIt5E9hyBHQKDdi7mjiVB7lWgmeERSVRqUVAPEJg XOScOGF3c3cdXnRWZAGFT6G3lwhWC0APAe9ewRXL+RScjZel28szU4Qki096KQooSoViOPReHbgT ZUfi59jk1ownIBng0yW6UEDLtHQja9OZEbE4fjT8UXo7yxKuccmZBzHKWmIM4Y+QYBqjjMV58jwN +Xcpt4JXCPF+6lxt9vgwzDNH4NnegHm/7CE9B3qKwB69BGPfTdqjekP5sJgNh2RSn96P/GKggqOV Np2t3KZHAz6HvGGJzDGq9I+ueSM9TtHBieN6+BmkZpWHdrcuu2KNrKCAZMsATCEzIMuaeE/fQktT X1clT4lSd29RBcBgkXqJMtyPoi/v9qqL0TlDDJl8vDbFzmTRVOw2joxmAJmO8Kg2MuAgyDXFT9hk QDkynFRXYgeLK41YWh1F5MzUelaHsMxsLyttoMd6ntvQMONjOf95rTNk2X7XhU4fi7hvU9wtswpx 0gU6RC0pmnmeEdGvBet/+owx+p/WKr6orvvszHavnCc3m5JhS+kyDBbZ50hoBApYCnUD1TgPoDFC IEuXseZ6i+R4W/Zp1nSBgEKO6NKMC7gAMA0HrJPx7qavIoIINhzPg9mY92j+FNA1R28OkIAHgB5H oAUAFAQVIlYduzYSbi96a/1KAMSPoX0H7U7fIOSdVP2JekJzC/MyIFodC3+x9T//+4u5IpwoSGLD X5WA --===============8931639458740056535==--