Alexander Nozdrin wrote:
> Hi Kristofer,
>
> the patch is Ok to push with some minor notes (as usual :)) -- see below.
>
> On Friday 07 December 2007 14:40, kpettersson@stripped wrote:
>
>> ChangeSet@stripped, 2007-12-07 12:40:08+01:00, thek@adventure.(none) +3 -0
>> Bug #27440 read_only allows create and drop database
>>
>> When read_only option was enabled, a user without SUPER privilege could
>> perform CREATE DATABASE and DROP DATABASE operations.
>>
>
> So, why did that happen? What was the technical cause of the user-visible
> problem? Could you please clarify.
>
>
It happened because the logic used to determine which flags to use and
which not to use were way to complex to understand at a glance. It is
not technical, it is stems from human error. I think it is in the change
set comment: What was wrong (from a functional stand point), how was it
fixed (a check was added). I don't understand what you want me to write.
> Just wondering. You're using '--' prefix for directives like "echo" and
> "error" and not using it for "connect", "disconnect" and "connection".
> Just wondering why? :) I'm not forcing anything here.
>
No particular reason. Why do you think it would be better to use '--' on
connect and disconnect?
>
>
>> + const my_bool user_is_super=
>> + ((long)(thd->security_ctx->master_access & SUPER_ACL) ==
>> + (long)SUPER_ACL);
>> +
>> + if (user_is_super)
>> + DBUG_RETURN(FALSE);
>>
>
> Well... The original code contained just
> "(thd->security_ctx->master_access & SUPER_ACL)" check.
> I wonder why do you need "user_is_super" and all these casts here?
> Again, Security_ctx::master_access is ulong; SUPER_ACL is long --
> it seems, casts are not needed here.
>
> Why not just:
>
> if (thd->security_ctx->master_access & SUPER_ACL)
> DBUG_RETURN(FALSE);
>
Fixed. The idea was to achieve explicit clarity, but it obviously failed. :)
>
>
>> + const my_bool create_temp_tables=
>> + (lex->sql_command == SQLCOM_CREATE_TABLE) &&
>> + (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE);
>> +
>> + const my_bool drop_temp_tables=
>> + (lex->sql_command == SQLCOM_DROP_TABLE) &&
>> + lex->drop_temporary;
>> +
>> + const my_bool update_real_tables=
>> + some_non_temp_table_to_be_updated(thd, all_tables) &&
>> + !(create_temp_tables || drop_temp_tables);
>> +
>> + const my_bool update_temp_tables=
>> + !update_real_tables;
>> +
>> + const my_bool create_or_drop_databases=
>> + (lex->sql_command == SQLCOM_CREATE_DB) ||
>> + (lex->sql_command == SQLCOM_DROP_DB);
>> +
>> + if (update_real_tables ||
>> + (update_temp_tables && create_or_drop_databases))
>>
>
> 1. I don't get why do you need "update_temp_tables" flag here?
> It seems, this expression is like:
>
> x || !x && y
>
> which is equialent to
>
> x || y
>
> so, why not just:
>
> if (update_real_tables || create_or_drop_databases)
>
> ?
>
> 2. Generally (and according to the coding style), the following code
>
> if (x)
> return true;
>
> return false;
>
> should be written as
>
> return x;
>
> So, I would write just
>
> return update_real_tables || create_or_drop_databases;
>
> but if you do believe the comments are useful, I think, you can leave
> things as is (i.e. keep the "if").
>
> That's it, thanks for working on this!
>
>
Again, the rewrites is to be able to make it easier for a human to
deduce the logic and the reasons behind. This effort is perhaps in vain,
but obviously, the original problem stems from too complex logical
statements. If you don't agree it has become more clear, I should
perhaps give up this idea and optimize it instead.
