- developers - lists.mariadb.org

Re: [Maria-developers] 95e1c46: MDEV-9898 SET ROLE NONE can crash mysqld.
by Sergei Golubchik 02 May '16

02 May '16

Hi, Alexey! On May 02, Alexey Botchkov wrote: > Fine. > But can you please explain the logic here? When connected to the server, > the user is authenticated with the > find_user_or_anon(). But when we check permissions for the SET ROLE NONE, > we use find_user_wild(). > Why is that? Because 'user' is thd->security_ctx->priv_user in this case, not thd->security_ctx->user. find_user_or_anon() works with user/host pair, using wildcard matching for both user and host values. While find_user_exact() works with priv_user/priv_host pairs, using exact matching for both user and host values. The very weird find_user_wild() works with inconsistent priv_user/host pairs, using wildcards only for the host, not for the user. This pair make no sense to me, but this is the historical MySQL behavior that I didn't risk to break :( It's MDEV-5233 Regards, Sergei

1 0

[Maria-developers] GSoC 2016: Introduction letter of Galina Shalygina
by Галина Шалыгина 01 May '16

01 May '16

Hi everyone! My name is Galina Shalygina and I was selected for GSoC to work with MariaDB company. I'm from St Petersburg, at Russia and I study at St Petersburg State University with major in Analytical Information Systems. I'm in my third year at the University. My nick on IRC is galina and on github it's shagalla. I've already worked with MariaDB Server, and my contribution on non-recursive CTE was accepted by MariaDB. Now I'm finishing and implementation of recursive CTE. My project for GsoC 2016 is called "Pushing conditions into non-mergeable views and derived tables in MariaDB ". I plan to implement the optimizations that push conditions into non-mergeable (such as grouping) views/derived tables. I will be mentored by igor_seattle and spetrunia. During the community bonding period I'm going to: 1. Clone server code and build it; 2. Read documentation; 3. Study all developments on my theme; 4. Start some kind of blog where I'll write reports during work period; 5. For this project I will need to be able to clone expressions, so I'll start to work on solving this problem. I'm looking forward to working on this project. It would be a great experience for me! Best wishes, Galina Shalygina.

1 0

Re: [Maria-developers] f9f290b: MDEV-9851: CREATE USER w/o IDENTIFIED BY clause causes crash when using cracklib plugin
by Sergei Golubchik 29 Apr '16

29 Apr '16

Hi, Nirbhay! On Mar 31, Nirbhay Choubey wrote: > revision-id: f9f290b6828eeb57cba611d006d2a9301dc52244 (mariadb-10.1.13-3-gf9f290b) > parent(s): f4d5fe277599da4549c97c660f324c88cf9a2542 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-03-31 18:03:44 -0400 > message: > > MDEV-9851: CREATE USER w/o IDENTIFIED BY clause causes crash when using cracklib plugin > > Add a check for NULL password. > > diff --git a/plugin/cracklib_password_check/cracklib_password_check.c b/plugin/cracklib_password_check/cracklib_password_check.c > index c593173..c192cdf 100644 > --- a/plugin/cracklib_password_check/cracklib_password_check.c > +++ b/plugin/cracklib_password_check/cracklib_password_check.c > @@ -33,7 +33,8 @@ static int crackme(MYSQL_LEX_STRING *username, MYSQL_LEX_STRING *password) > if ((host= strchr(user, '@'))) > *host++= 0; > > - if ((res= FascistCheckUser(password->str, dictionary, user, host))) > + if ((password->str == NULL) || // No password > + (res= FascistCheckUser(password->str, dictionary, user, host))) > { > my_printf_error(ER_NOT_VALID_PASSWORD, "cracklib: %s", > MYF(ME_JUST_WARNING), res); You forgot to fix the simple_password_check plugin. And if all plugins need to do the same check - it's a strong indication that this should've been done in the server. So, please, fix this in sql_acl.cc instead. Like this: - struct validation_data data= { &user->user, &user->pwtext }; + struct validation_data data= { &user->user, user->pwtext.str ? &user->pwtext : &empy_lex_str }; Ok to push with this fix and your test case. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

Re: [Maria-developers] 95e1c46: MDEV-9898 SET ROLE NONE can crash mysqld.
by Sergei Golubchik 29 Apr '16

29 Apr '16

Hi, Alexey! On Apr 28, Alexey Botchkov wrote: > revision-id: 95e1c46c1709e7244332a025a527cacceab7da60 (mariadb-10.1.13-14-g95e1c46) > parent(s): 646c4cea58afbb369021a3d7b5ccbbf6aed708d4 > committer: Alexey Botchkov > timestamp: 2016-04-28 13:36:36 +0400 > message: > > MDEV-9898 SET ROLE NONE can crash mysqld. > The check_user_can_set_role() used find_user_exact() to get the > permissions for the SET ROLE NONE command. > Which returned NULL too often, for instance when user > authenticated as 'user'@'%'. > Now we use find_user_or_anon() instead. No, this is wrong. SET ROLE NONE should use the same rule as SET ROLE name. And the latter uses acl_user->wild_eq(user, host, ip). Thus SET ROLE NONE should find the user with ACL_USER::wild_eq, that is, it should use find_user_wild(). Ok to push with find_user_wild(). Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MariaDB packages for Alpine Linux
by Guillaume Lefranc 29 Apr '16

29 Apr '16

Hello, there is an outstanding request in docker-library/mariadb for Alpine support: https://github.com/docker-library/mariadb/issues/53 What I know is that we currently don't build packages for Alpine. Alpine is going to be the platform of choice for Docker containers in the future (you can read about the various advantages of Alpine here: https://www.brianchristner.io/docker-is-moving-to-alpine-linux/) Federico Razzoli has managed to make MariaDB 10.1 work in Alpine with some caveats (no tokudb, no oqgraph, no galera). You can look at his work here https://hub.docker.com/r/santec/mariadb-alpine/. I admit I don't know about portability of Galera lib on Alpine - should probably talk to Codership about that. Is there any way we can make this happen?

1 0

[Maria-developers] Why reserved keywords: UTC_TIME, UTC_DATE, UTC_DATETIME
by Alexander Barkov 29 Apr '16

29 Apr '16

Hello, I noticed that UTC_TIME, UTC_DATE, UTC_DATETIME are reserved keywords: MariaDB [test]> SELECT * FROM utc_time; ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'utc_time' at line 1 I think they should not. - These keywords are not mentioned in the SQL standard - They are not involved in any complex grammar that would prevent them from being non-reserved keywords Looks like a bug... Any objections to make them non-reserved keywords in 10.2?

6 8

Re: [Maria-developers] GSoC 2016: Unique indexes for blobs
by Sergei Golubchik 27 Apr '16

27 Apr '16

Hi, Shubham! On Apr 24, Shubham Barai wrote: > Hi Sergei, > > I have created github repository with a new branch for my prototype. Great, what's the url? btw, please, use "Reply to all", so that the whole list could see your progress. Also google recommends you to blog regularly about what you're doing: http://write.flossmanuals.net/gsocstudentguide/time-management-for-students/ but if you don't like that, you can send weekly progress report emails instead. Whatever suits you best. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 6

[Maria-developers] GSoC Student Introduction
by Charles Muurmu 27 Apr '16

27 Apr '16

Hi everyone, I am Charles Muurmu also known as laserlight on IRC and I was selected for GSoC 2016 to work on migrating MariaDB Cassandra storage engine from the Thrift API to Datastax driver. I will be mentored by Spetrunia. My goals for this community bonding period will be get myself ready by accomplishing the following tasks: Fork and clone the server code. Try to build the code. Setup a blog on which I will post weekly reports on progress. Study the Datastax C++ driver API and documentation. Study the ha_cassandra.xx code and storage engines architecture. I believe these will prepare me for my first task when coding begins which will be to integrate the Datastax C++ driver into the MariaDB Server build system. I equally look forward to more instructions from my Mentor on how we will work on this task and lastly, I would love to congratulate the other students selected with MariaDB. Cheers, laserlight.

1 0

Re: [Maria-developers] [Commits] c204e9b: MDEV-9610 Trigger on normal table can't insert into CONNECT engine table - Access Denied
by Alexander Barkov 26 Apr '16

26 Apr '16

Hi Sergei, On 04/25/2016 08:42 PM, serg(a)mariadb.org wrote: > revision-id: c204e9bd63e7f6ce40e83b1f7fc0b4607a131009 (mariadb-10.0.24-43-gc204e9b) > parent(s): ce38adddfa91949b30956abd0e4cab201effcdef > author: Sergei Golubchik > committer: Sergei Golubchik > timestamp: 2016-04-25 18:41:31 +0200 > message: > > MDEV-9610 Trigger on normal table can't insert into CONNECT engine table - Access Denied > > in case of prelocking, don't check table->grant.privilege > in handler::external_lock(), do it in > handler::start_stmt(). > > --- > storage/connect/ha_connect.cc | 58 ++++++++++++++++++++-- > storage/connect/ha_connect.h | 1 + > storage/connect/mysql-test/connect/r/grant3.result | 5 ++ > storage/connect/mysql-test/connect/t/grant3.test | 11 ++++ > 4 files changed, 70 insertions(+), 5 deletions(-) > > diff --git a/storage/connect/ha_connect.cc b/storage/connect/ha_connect.cc > index 645d000..1032567 100644 > --- a/storage/connect/ha_connect.cc > +++ b/storage/connect/ha_connect.cc > @@ -4054,6 +4054,14 @@ int ha_connect::delete_all_rows() > } // end of delete_all_rows > > > +static void issue_access_denied_error(THD *thd) > +{ > + status_var_increment(thd->status_var.access_denied_errors); > + my_error(access_denied_error_code(thd->password), MYF(0), > + thd->security_ctx->priv_user, thd->security_ctx->priv_host, > + (thd->password ? ER(ER_YES) : ER(ER_NO))); > +} > + > bool ha_connect::check_privileges(THD *thd, PTOS options, char *dbn) > { > const char *db= (dbn && *dbn) ? dbn : NULL; > @@ -4124,12 +4132,9 @@ bool ha_connect::check_privileges(THD *thd, PTOS options, char *dbn) > */ > if (!table || !table->mdl_ticket || table->mdl_ticket->get_type() == MDL_EXCLUSIVE) > return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0); > - if (table->grant.privilege & FILE_ACL) > + if (thd->lex->requires_prelocking() || table->grant.privilege & FILE_ACL) > return false; > - status_var_increment(thd->status_var.access_denied_errors); > - my_error(access_denied_error_code(thd->password), MYF(0), > - thd->security_ctx->priv_user, thd->security_ctx->priv_host, > - (thd->password ? ER(ER_YES) : ER(ER_NO))); > + issue_access_denied_error(thd); > return true; > > // This is temporary until a solution is found > @@ -4146,6 +4151,46 @@ bool ha_connect::check_privileges(THD *thd, PTOS options, char *dbn) > return true; > } // end of check_privileges > > +bool ha_connect::check_cached_privileges(THD *thd) > +{ > + PTOS options= GetTableOptionStruct(); > + TABTYPE type=GetRealType(options); > + > +#ifdef NO_EMBEDDED_ACCESS_CHECKS > + return false; > +#endif > + > + switch (type) { > + case TAB_DOS: > + case TAB_FIX: > + case TAB_BIN: > + case TAB_CSV: > + case TAB_FMT: > + case TAB_DBF: > + case TAB_XML: > + case TAB_INI: > + case TAB_VEC: > + case TAB_JSON: > + if (!options->filename || !*options->filename) > + return false; > + /* Fall through to check FILE_ACL */ > + case TAB_ODBC: > + case TAB_MYSQL: > + case TAB_DIR: > + case TAB_MAC: > + case TAB_WMI: > + case TAB_OEM: > + if (table->grant.privilege & FILE_ACL) > + return false; > + issue_access_denied_error(thd); > + return true; > + default: The patch looks fine. I'd suggest two things. 1. Don't use "default" Can you please list all possible type value instead of "default"? Otherwise, when a new table type is added the next time, it will be easier to forget to update check_cached_privileges() accordingly. 2. Try to avoid duplicate code. Btw, I'd probably try to remove the duplicate switch, by adding a new method like this: enum Extra_acl { CONNECT_SECURE_FILE_PATH= 1, CONNECT_FILE_ACL= 1<<1; } int ha_connect::extra_acl_needed(THD *thd, PTOS options) { ... switch (GetRealType(options)) { ... case TAB_DOS: case TAB_FIX: case TAB_BIN: case TAB_CSV: case TAB_FMT: case TAB_DBF: case TAB_XML: case TAB_INI: case TAB_VEC: case TAB_JSON: return (options->filename && *options->filename) ? CONNECT_SECURE_FILE_PATH : 0; case TAB_ODBC: case TAB_MYSQL: case TAB_DIR: case TAB_MAC: case TAB_WMI: case TAB_OEM: #ifdef NO_EMBEDDED_ACCESS_CHECKS return 0; #endif return CONNECT_FILE_ACL; ... } ... } And then reuse it in both ha_connect::check_privilege() and ha_connect::check_cached_privileges(). The latter would look about like this: bool ha_connect::check_cached_privileges(THD *thd) { PTOS options= GetTableOptionStruct(); if (!(extra_acl_needed(options) & CONNECT_FILE_ACL) || (table->grant.privilege & FILE_ACL)) return false; issue_access_denied_error(thd); return true; } > + return false; > + } > + return false; > + > +} // end of check_cached_privileges > + > // Check that two indexes are equivalent > bool ha_connect::IsSameIndex(PIXDEF xp1, PIXDEF xp2) > { > @@ -4308,6 +4353,9 @@ int ha_connect::start_stmt(THD *thd, thr_lock_type lock_type) > PGLOBAL g= GetPlug(thd, xp); > DBUG_ENTER("ha_connect::start_stmt"); > > + if (check_cached_privileges(thd)) > + DBUG_RETURN(HA_ERR_INTERNAL_ERROR); > + > // Action will depend on lock_type > switch (lock_type) { > case TL_WRITE_ALLOW_WRITE: > diff --git a/storage/connect/ha_connect.h b/storage/connect/ha_connect.h > index 05cc872..fae804b 100644 > --- a/storage/connect/ha_connect.h > +++ b/storage/connect/ha_connect.h > @@ -537,6 +537,7 @@ int index_prev(uchar *buf); > > protected: > bool check_privileges(THD *thd, PTOS options, char *dbn); > + bool check_cached_privileges(THD *thd); It looks like the number of leading spaces is wrong here. > MODE CheckMode(PGLOBAL g, THD *thd, MODE newmode, bool *chk, bool *cras); > char *GetDBfromName(const char *name); > > diff --git a/storage/connect/mysql-test/connect/r/grant3.result b/storage/connect/mysql-test/connect/r/grant3.result > new file mode 100644 > index 0000000..2f9d37b > --- /dev/null > +++ b/storage/connect/mysql-test/connect/r/grant3.result > @@ -0,0 +1,5 @@ > +create table tcon (i int) engine=Connect table_type=DOS file_name='tcon.dos'; > +create table tin (i int); > +create trigger tr after insert on tin for each row insert into tcon values (new.i); > +insert into tin values (1); > +drop table tin,tcon; > diff --git a/storage/connect/mysql-test/connect/t/grant3.test b/storage/connect/mysql-test/connect/t/grant3.test > new file mode 100644 > index 0000000..9f05ca7 > --- /dev/null > +++ b/storage/connect/mysql-test/connect/t/grant3.test > @@ -0,0 +1,11 @@ > +# > +# MDEV-9610 Trigger on normal table can't insert into CONNECT engine table - Access Denied > +# > +create table tcon (i int) engine=Connect table_type=DOS file_name='tcon.dos'; > +create table tin (i int); > +create trigger tr after insert on tin for each row insert into tcon values (new.i); > +insert into tin values (1); > +drop table tin,tcon; > + > +let datadir=`select @@datadir`; > +remove_file $datadir/test/tcon.dos; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits >

2 1

Re: [Maria-developers] Deadlock with STOP SLAVE and LOCK_active_mi
by Michael Widenius 26 Apr '16

26 Apr '16

Hi! >>> Any suggestions for how this is supposed to work? Or is it just broken >>> by design, but saved because normally slave threads do not need to >>> access SHOW STATUS or system variables? <cut> > Hm. So I went through all of the code that references LOCK_active_mi to try > and understand this. It seems there are two main uses: > > 1. As you say, to serialise admin commands. I think the list is: end_slave() > (server shutdown); CHANGE MASTER; START/STOP SLAVE; START/STOP ALL SLAVES; > RESET SLAVE; FLUSH SLAVE. > > 2. To protect access to master_info_index, eg. to prevent a Master_info from > disappearing while it is accessed. When reading about this, I think it would be better to have a counter in Master_info if someone is using it and only delete if if the counter is zero. Would be trivial to add an increment of the counter in get_master_info(). The counter could either be protected by the LOCK_active_mi or one of the mutexes in Master_info like data_lock. > There is a comment in rpl_rli.h that LOCK_active_mi protects > Relay_log_info::inited, but I did not understand it - maybe it is wrong? Setting inited is indirectly protected by LOCK_active_mi, as init_all_master_info() which calls init_master_info(), is protected by LOCK_active_mi. Looking at the code, I didn't however see any need to protect inited as this is an internal flag that is always 1 after start. It's main usage is to avoid some re-initialization of relay logs on retries. > So the idea would be to make a new lock for (1), and keep LOCK_active_mi for > (2). > > Actually, using a lock for (1) seems a bad idea - STOP SLAVE can take a long > time to run, and a mutex makes it unkillable. A condition variable might be > better (to make STOP SLAVE killable). But I guess that is a separate issue, > it would not affect the possibility of deadlocks. Agree that a condition variable would be better, but not critical as one has already a problem if stop_slave doesn't work. > Both LOCK_active_mi and the new LOCK_serialize_replication_admin_commands > must by themselves prevent master_info_index from having elements added or > removed. This is needed for start_all_slaves() and stop_all_slaves() to work > correctly, I think. Why both? Isn't it enough to first take LOCK_serialize_replication_admin_commands and then take LOCK_active_mi if one needs to access master_info_index ? > Something will need to be done for remove_master_info(). Currently, it also > tries to stop slave threads (from within free_key_master_info()) while > holding LOCK_active_mi. It seems that threads should be stopped before > attempting to remove their mi from master_info_index, probably? To do that we would need to: - Add a flag in Master_info that it's not usable anymore and change this flag only under LOCK_active_mi. get_master_info() could wait (on a condition) if this flag is set. - Add a counter that Master_info is in use. - Add a function to release Master_info. - Call terminate_slave_threads() outside of free_key_master_info() > Actually, there would still be the deadlock problem after introducing the > new lock, because it is possible for a slave thread to run CHANGE MASTER! > (and I think START SLAVE / RESET SLAVE as well). But that is probably a bug, > does not seem good that this is possible. I guess these need a check to > throw an ER_LOCK_OR_ACTIVE_TRANSACTION error, like STOP SLAVE. Wouldn't the new lock LOCK_serialize_replication_admin_commands fix that? > I think I also found another lock problem while reading the code, for > MASTER_POS_WAIT() og SHOW BINLOG EVENTS. They both grab a Master_info under > LOCK_active_mi, but then proceed to access it after releasing the lock. I do > not see anything that prevents the Master_info to disappear under their feet > if RESET SLAVE ALL is run in another thread? But again, that's a separate > issue, I suppose. Probably there are some other tricky deadlock issues > lurking here. I checked the function mysql_show_binlog_events() but could not find any access to Master_info after mysql_mutex_unlock(&LOCK_active_mi) was released. > I don't know. It seems splitting into a new > LOCK_serialize_replication_admin_commands could solve the deadlock issue, > maybe, if the other problems mentioned above are addressed. It seems very > hard to convince oneself that this would not introduce new problems > somewhere, the locking in replication is really complex and feels very > fragile :-/ It doesn't really feel like something one would want to do in > 10.1 (certainly not 10.0), but maybe 10.2? > > It's kind of a very edge-case problem (and seems to have been there for a > _long_ time). Still, hanging the mysqld process hard, requiring kill -9, is > also not nice... Adding two new flags, one if master_info is in use and one if it's unusable, shouldn't be that hard to make reasonable safe in 10.1 I am now back in Finland and working. Feel free to call me to discuss this any time. Regards, Monty

2 2