MDEV-6733: Add DEFINER support to ALTER PROCEDURE#5060
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the ability to change the definer of a stored procedure using the ALTER PROCEDURE statement. The changes include updates to the parser, the routine update handler, and the addition of privilege checks. Feedback includes addressing a potential buffer overflow when handling user/host strings, utilizing existing helper functions for more robust privilege and hostname checks, ensuring the procedure cache is invalidated only upon successful updates, and correcting a type mismatch in the grammar definition.
| char definer_str[USER_HOST_BUFF_SIZE]; | ||
| uint definer_len= strxmov(definer_str, definer->user.str, "@", | ||
| definer->host.str, NullS) - | ||
| definer_str; | ||
| table->field[MYSQL_PROC_FIELD_DEFINER]->set_notnull(); | ||
| table->field[MYSQL_PROC_FIELD_DEFINER]->store(definer_str, definer_len, | ||
| system_charset_info); |
There was a problem hiding this comment.
Using a fixed-size buffer of USER_HOST_BUFF_SIZE (142 bytes) is unsafe. In modern MariaDB, usernames can be up to 128 characters, which in UTF-8 can exceed this buffer size when combined with the hostname. This could lead to a buffer overflow. It is safer to use the String class which handles dynamic allocation and correctly manages lengths.
String definer_str;
definer_str.append(definer->user.str, definer->user.length);
definer_str.append('@');
definer_str.append(definer->host.str, definer->host.length);
table->field[MYSQL_PROC_FIELD_DEFINER]->set_notnull();
table->field[MYSQL_PROC_FIELD_DEFINER]->store(definer_str.ptr(),
definer_str.length(),
system_charset_info);| if (lex->definer) | ||
| { | ||
| if ((!lex_string_eq(&lex->definer->user, thd->security_ctx->priv_user, strlen(thd->security_ctx->priv_user)) || | ||
| !lex_string_eq(&lex->definer->host, thd->security_ctx->priv_host, strlen(thd->security_ctx->priv_host))) && | ||
| check_global_access(thd, SET_USER_ACL | SUPER_ACL)) | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Instead of manually implementing the privilege check, you should use the existing check_definer_access() function. This improves maintainability and fixes a bug in the current implementation: lex_string_eq is case-sensitive, but hostnames in MariaDB should be compared case-insensitively. check_definer_access() handles this correctly using my_strcasecmp for the host part.
if (lex->definer && check_definer_access(thd, lex->definer))
return 1;| table->field[MYSQL_PROC_FIELD_DEFINER]->set_notnull(); | ||
| table->field[MYSQL_PROC_FIELD_DEFINER]->store(definer_str, definer_len, | ||
| system_charset_info); | ||
| sp_cache_invalidate(); |
There was a problem hiding this comment.
The call to sp_cache_invalidate() should ideally be placed after the successful execution of ha_update_row(). This ensures the cache is only invalidated if the database record was actually updated. Furthermore, consider if the cache should be invalidated for any characteristic change (like COMMENT), as the cached sp_head object contains these values and might become stale.
| %type <lex_user> user grant_user grant_role user_or_role current_role | ||
| admin_option_for_role user_maybe_role role_name | ||
| user_name | ||
| user_name sp_a_chistic |
There was a problem hiding this comment.
Declaring sp_a_chistic as <lex_user> is technically incorrect because the sp_chistic branch of this rule does not return a LEX_USER* value. Since the value of sp_a_chistic is not used by the parent sp_a_chistics rule, it should remain untyped to avoid potential issues with uninitialized values in the parser.
user_name
gkodinov
added
the
External Contribution
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please:
- Add a commit message to your commit that is compliant with the MariaDB server's coding standards.
- Make sure all buildbot tests are passing.
- Make sure the pull request implements everything the mdev is talking about. In your case it doesn't: there's no support or testing for triggers, events and functions.
Also see the below for some additional suggestions.
| @@ -0,0 +1,20 @@ | |||
| --echo # ALTER PROCEDURE DEFINER | |||
There was a problem hiding this comment.
We customarily start with a prefix comment like
--echo MDEV-1234 this mdev does so and so
|
|
||
| ALTER PROCEDURE p1 DEFINER = 'user1'@'localhost'; | ||
|
|
||
| --error ER_SPECIFIC_ACCESS_DENIED_ERROR |
There was a problem hiding this comment.
Please provide a design document someplace: what is supposed to work and what isn't. This should ideally be into the MDEV itself.
| return 1; | ||
| if (lex->definer) | ||
| { | ||
| if ((!lex_string_eq(&lex->definer->user, thd->security_ctx->priv_user, strlen(thd->security_ctx->priv_user)) || |
There was a problem hiding this comment.
I believe this is a bad design decision: only allow changing definers for your procedures.
The proper (ideal) way to do this is at multiple levels (IMHO):
- a specific privilege granted to somebody at global level should allow unrestricted owner change
- a specific privilege at database level should allow unrestricted privilege owner changes for the full database
- a specific privilege on a stored procedure or function or trigger should allow unrestricted privilege owner changes for the full database.
- there can be a different privilege allowing one to set another specific account as owner, which when granted, will allow one to grant the account as owner.
Anyways, it should be privilege based.
| { | ||
| if ((!lex_string_eq(&lex->definer->user, thd->security_ctx->priv_user, strlen(thd->security_ctx->priv_user)) || | ||
| !lex_string_eq(&lex->definer->host, thd->security_ctx->priv_host, strlen(thd->security_ctx->priv_host))) && | ||
| check_global_access(thd, SET_USER_ACL | SUPER_ACL)) |
There was a problem hiding this comment.
never re-use privileges like that. This turns them into roles.
| */ | ||
| /* Conditionally writes to binlog */ | ||
| sp_result= sph->sp_update_routine(thd, lex->spname, &lex->sp_chistics); | ||
| sp_result= sph->sp_update_routine(thd, lex->spname, &lex->sp_chistics, lex->definer); |
There was a problem hiding this comment.
this is where tests fail with a crash in buildbot:
e.g.:
main.sp w3 [ fail ]
Test ended at 2026-05-10 10:17:04
CURRENT_TEST: main.sp
mysqltest: At line 860: query 'alter function chistics
no sql
comment 'Characteristics function test'' failed: <Unknown> (2013): Lost connection to server during query
The result from queries just before the failure was:
< snip >
drop procedure chistics|
create function chistics() returns int
language sql
deterministic
sql security invoker
comment 'Characteristics procedure test'
return 42|
show create function chistics|
Function sql_mode Create Function character_set_client collation_connection Database Collation
chistics STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION CREATE DEFINER=`root`@`localhost` FUNCTION `chistics`() RETURNS int(11)
DETERMINISTIC
SQL SECURITY INVOKER
COMMENT 'Characteristics procedure test'
return 42 latin1 latin1_swedish_ci utf8mb4_uca1400_ai_ci
select chistics()|
chistics()
42
alter function chistics
no sql
comment 'Characteristics function test'|
More results from queries before failure can be found in /home/buildbot/aarch64-debian-11/build/mysql-test/var/3/log/sp.log
Server [mysqld.1 - pid: 60505, winpid: 60505, exit: 256] failed during test run
Server log from this test:
----------SERVER LOG START-----------
260510 10:17:03 [ERROR] /home/buildbot/aarch64-debian-11/build/sql/mariadbd got signal 11 ;
Sorry, we probably made a mistake, and this is a bug.
Your assistance in bug reporting will enable us to fix this for the next release.
To report this bug, see https://mariadb.com/kb/en/reporting-bugs about how to report
a bug on https://jira.mariadb.org/.
Please include the information from the server start above, to the end of the
information below.
Server version: 13.0.1-MariaDB-log source revision: c579a5878830909e07ba9c45778defe351d6672b
The information page at https://mariadb.com/kb/en/how-to-produce-a-full-stack-trace-for-mariadbd/
contains instructions to obtain a better version of the backtrace below.
Following these instructions will help MariaDB developers provide a fix quicker.
Attempting backtrace. Include this in the bug report.
(note: Retrieving this information may fail)
Thread pointer: 0xffff78000c68
stack_bottom = 0xffff94978000 thread_stack 0x49000
mysys/stacktrace.c:216(my_print_stacktrace)[0xaaaae2297b50]
sql/signal_handler.cc:230(handle_fatal_signal)[0xaaaae1dfa52c]
addr2line: 'linux-vdso.so.1': No such file
linux-vdso.so.1(__kernel_rt_sigreturn+0x0)[0xffff9c8787bc]
strings/strxmov.c:53(strxmov)[0xaaaae22ee150]
sql/sp.cc:1779(Sp_handler::sp_update_routine(THD*, Database_qualified_name const*, st_sp_chistics const*, LEX_USER*) const)[0xaaaae1b07a74]
sql/sql_parse.cc:6466(alter_routine(THD*, LEX*))[0xaaaae1ba7824]
sql/sql_parse.cc:5667(mysql_execute_command(THD*, bool))[0xaaaae1badd48]
sql/sql_parse.cc:7966(mysql_parse(THD*, char*, unsigned int, Parser_state*))[0xaaaae1bb1830]
sql/sql_parse.cc:1900(dispatch_command(enum_server_command, THD*, char*, unsigned int, bool))[0xaaaae1bb340c]
sql/sql_parse.cc:1432(do_command(THD*, bool))[0xaaaae1bb5018]
sql/sql_connect.cc:1503(do_handle_one_connection(CONNECT*, bool))[0xaaaae1cb7bf4]
sql/sql_connect.cc:1419(handle_one_connection)[0xaaaae1cb7fc0]
perfschema/pfs.cc:2201(pfs_spawn_thread)[0xaaaae200fd4c]
/lib/aarch64-linux-gnu/libpthread.so.0(+0x7648)[0xffff9c2bb648]
/lib/aarch64-linux-gnu/libc.so.6(+0xd1c9c)[0xffff9bf54c9c]
2 participants
This PR implements the ALTER PROCEDURE ... DEFINER = ... syntax