Hi Sergei, On 04/25/2016 08:42 PM, serg@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@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits