maria-developers team mailing list archive

[Alexey Botchkov <holyfoot@xxxxxxxxxxx>]

> What security context are you using now?
> root? whatever happen to be in the THD?
> What if you create a new THD?

The thd->security_ctx->skip_grants() is done for the created THD.
Otherwise yes, the current context is used.
Going to fix that with the local empty context.

> Feel free to replace it with any other macro that is
> guaranteed to be set for the server code.
Since at the moment it only affects the SQL service plugin, i'd keep it as
you did.
Just adding the comment.
If something else starts to depend on it, then we'll better add a specific
define for it.


> > +  thd->set_time();

> why? May be it should be executed using the timestamp of the top-level
> statement?

When that can be desirable?
I did that thinking 'what if the set_time() for the current thread wasn't
called at all'.
Though removed this for now.

So the patch with the abovementioned changes.
9ad45b41d385fca8215880cce1757ebe7f2396eb

Best regards.
HF


On Fri, Sep 10, 2021 at 7:54 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Alexey!
>
> On Sep 10, Alexey Botchkov wrote:
> > > For example, I'd think it connects to the current server, internally.
> > > But then, why does it need host and user?
> >
> > Initially i thought it can be put in the thread's security_ctx so
> functions
> > like CURRENT_USER
> > return these values. After some more meditation i decided that it
> > doesn't seem to be useful and just removed these arguments.
>
> What security context are you using now?
> root? whatever happen to be in the THD?
> What if you create a new THD?
>
> > > > +ADD_DEFINITIONS(-DMYSQL_SERVER)
> > > Why?
> > Couldn't figure out how to get rid of it.
> > Well your trick with the '#if !defined(MYSQL_SERVICE_SQL)' in the mysql.h
> > helps.
> > I wasn't bold enough to mention the particular service in such an exposed
> > file as mysql.h :)
>
> Feel free to replace it with any other macro that is guaranteed to be
> set for the server code (including plugins and anything that doesn't
> define MYSQL_SERVER, but still is in the server). Any plugin related
> define should work, I think.
>
> > > > +#include <mysql.h>
> > > shouldn't be needed
> > My idea was that those not using the SQL service don't have to see the
> > 'mysql.h' declarations.
> > Now all the plugins see all these mysql_xxx() functions and related
> > structures. Not sure if it's good.
>
> I simply mean that you already include mysql.h in the service_sql.h.
>
> If you wouldn't, then yes, a separate include would be needed.
>
> > The update pushed 71338ddada9f331af57b737a586f56dc6f2e483f.
>
> Thanks. One unanswered question below:
>
> > diff --git a/sql/sql_class.h b/sql/sql_class.h
> > index e569fcd32d6..964626be3d4 100644
> > --- a/sql/sql_class.h
> > +++ b/sql/sql_class.h
> > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> > index 09ad632dd98..4725855c130 100644
> > --- a/sql/sql_prepare.cc
> > +++ b/sql/sql_prepare.cc
> > @@ -4857,6 +4870,7 @@
> Prepared_statement::execute_server_runnable(Server_runnable
> *server_runnable)
> >    if (!(lex= new (mem_root) st_lex_local))
> >      return TRUE;
> >
> > +  thd->set_time();
>
> why? May be it should be executed using the timestamp of the top-level
> statement?
>
> >    thd->set_n_backup_statement(this, &stmt_backup);
> >    thd->set_n_backup_active_arena(this, &stmt_backup);
> >    thd->stmt_arena= this;
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
>