Also, as a stylistic / readability change, can you please rename all occurrences of your String str to be: String db_and_table.
Ok to push after this.
Vicențiu Ciorbaru
added a comment - Hi Anel!
Looks like you forgot the null pointer check like I told you here:
https://mariadb.zulipchat.com/#narrow/stream/252587-AskMonty/topic/MDEV-28548/near/282130265
Here is my suggestion:
@@ -7
763,9 +7763,13 @@ bool check_grant(THD *thd, ulong want_access, TABLE_LIST *tables,
status_var_increment(thd->status_var.access_denied_errors);
String str;
- str.append(tl->get_db_name());
- str.append('.');
- str.append(tl->get_table_name());
+ if (tl)
+ {
+ str.append(tl->get_db_name());
+ str.append('.');
+ str.append(tl->get_table_name());
+ }
+
Also, as a stylistic / readability change, can you please rename all occurrences of your String str to be: String db_and_table .
Ok to push after this.
1. 'db.table' quoting is always wrong, it doesn't escape characters properly and doesn't produce a valid identifier
2. using a helper String makes sense only if the value is different in different invocations. if it's always 'db.table' — it should be in the error message
Sergei Golubchik
added a comment - It should be fixed like
--- a/sql/share/errmsg-utf8.txt
+++ b/sql/share/errmsg-utf8.txt
@@ -3250,7 +3250,7 @@ ER_TABLEACCESS_DENIED_ERROR 42000
cze "%-.100T příkaz nepřístupný pro uživatele: '%s'@'%s' pro tabulku '>
dan "%-.100T-kommandoen er ikke tilladt for brugeren '%s'@'%s' for tab>
nla "%-.100T commando geweigerd voor gebruiker: '%s'@'%s' voor tabel '>
- eng "%-.100T command denied to user '%s'@'%s' for table '%-.192s'"
+ eng "%-.100T command denied to user '%s'@'%s' for table %`s.%`s'"
jps "コマンド %-.100T は ユーザー '%s'@'%s' ,テーブル '%-.192s' に対し>
est "%-.100T käsk ei ole lubatud kasutajale '%s'@'%s' tabelis '%-.192s>
fre "La commande '%-.100T' est interdite à l'utilisateur: '%s'@'%s' su>
--- a/sql/sql_acl.cc
+++ b/sql/sql_acl.cc
@@ -7051,7 +7051,8 @@ int mysql_table_grant(THD *thd, TABLE_LIST *table_list,
table_list->grant.want_privilege);
my_error(ER_TABLEACCESS_DENIED_ERROR, MYF(0),
command, thd->security_ctx->priv_user,
- thd->security_ctx->host_or_ip, table_list->alias.str);
+ thd->security_ctx->host_or_ip, table_list->db.str,
+ table_list->alias.str);
DBUG_RETURN(-1);
}
}
1. 'db.table' quoting is always wrong, it doesn't escape characters properly and doesn't produce a valid identifier
2. using a helper String makes sense only if the value is different in different invocations. if it's always 'db.table' — it should be in the error message
serg You are right with both points and for point #2 I had considered it, but I wrongly drew the conclusion that we can't change the string message in stable releases. I now realize that this restriction is about adding error messages, not about modifying existing ones.
anel note that Serg's proposed patch, which I agree with means you need to update the translation string for all languages to use the "escaping" syntax with 2 identifiers.
Vicențiu Ciorbaru
added a comment - - edited serg You are right with both points and for point #2 I had considered it, but I wrongly drew the conclusion that we can't change the string message in stable releases. I now realize that this restriction is about adding error messages, not about modifying existing ones.
anel note that Serg's proposed patch, which I agree with means you need to update the translation string for all languages to use the "escaping" syntax with 2 identifiers.
Hi,
I knew that this could be solution but I didn't know when it is acceptable to change the error message.
I knew we cannot add the error messages to the stable releases, so followed the same logic for changing them.
Anel Husakovic
added a comment - Hi,
I knew that this could be solution but I didn't know when it is acceptable to change the error message.
I knew we cannot add the error messages to the stable releases, so followed the same logic for changing them.
Minor MDEV-28455 CREATE TEMPORARY TABLES privilege is insufficient for SHOW COLUMNS
Hi Anel!
Looks like you forgot the null pointer check like I told you here:
https://mariadb.zulipchat.com/#narrow/stream/252587-AskMonty/topic/MDEV-28548/near/282130265
Here is my suggestion:
@@ -7763,9 +7763,13 @@ bool check_grant(THD *thd, ulong want_access, TABLE_LIST *tables,status_var_increment(thd->status_var.access_denied_errors);String str;- str.append(tl->get_db_name());- str.append('.');- str.append(tl->get_table_name());+ if (tl)+ {+ str.append(tl->get_db_name());+ str.append('.');+ str.append(tl->get_table_name());+ }Also, as a stylistic / readability change, can you please rename all occurrences of your String str to be: String db_and_table.
Ok to push after this.