maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 07/24/2015 10:55 PM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> On Jul 24, Alexander Barkov wrote:
> > Hi Sergei,
> >
> > Sorry for delay, I was busy with 10.1 issues.
>
> That's perfectly fine. And even good - there's little sense to spend
> time on 10.0 bugs when the next release in line is 10.1.
>
> > Thanks for review. A new patch is attached.
> > This is a major rewrite since last time.
> > I think the code now looks much easier to understand.
>
> Not really, I couldn't understand what you're trying to do this time :(
> Besides it's 32K of changes in 10.0-GA, and for what? Corner case
> CONNECT issue. Could you try to come up with a less intrusive solution,
> please?

Let me try to explain. The idea is very simple, really.


Calling check_access() inside external_lock() is a bad idea:

- it's simply not correct in case of VIEW or SP,
  because it checks privileges for the current INVOKER
  and ignores the "SQL SECURITY DEFINER" clause.

- and by the way, it's slow, and if it's already done in sql_oarse.cc,
  why call it for the  second time? It's nice to reuse the value
  returned in the first call of check_access().


The correct privilege value that takes into account "SQL SECURITY 
DEFINER" becomes known in sql_parse.cc, in mysql_execute_command()
and its callees, like insert_precheck() etc.


So I'm trying to make this correct privilege value be consistently 
available in external_lock() for all SQLCOM_XXXX commands by
assigning it to table->grant.privilege.
Before the patch, table->grant.privilege is just set to 0 in many cases
and therefore is useless in external_lock().


In order to initialize table->grant.privilege properly in all cases,
I added a new parameter "ulong privilege" to these functions:

ha_create_table()
open_table_uncached()
open_table_from_share().

The idea is to make the correct privilege value available at the point
where a TABLE instance is initialized and assign this value to 
table->grant.privilege instead of 0.

Does this explanation help?


The patch looks quite safe for 10.0 for me:

It does not seem to affect the other code, except ConnectSE.
It should not be harmful to have the real privilege value instead of 0
in table->grant.privilege. Or, can it be harmful somehow?

Thanks.

> Regards,
> Sergei