Skip to content

MDEV-6733: Add DEFINER support to ALTER PROCEDURE #5060

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
amrSherif12 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MDEV-6733: Add DEFINER support to ALTER PROCEDURE #5060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions mysql-test/main/alter_procedure_definer.result
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# ALTER PROCEDURE DEFINER
CREATE USER 'user1'@'localhost';
GRANT ALL PRIVILEGES ON test.* TO 'user1'@'localhost';
connect con1, localhost, user1, , test;
CREATE PROCEDURE p1() SELECT 1;
ALTER PROCEDURE p1 DEFINER = 'user1'@'localhost';
ALTER PROCEDURE p1 DEFINER = 'root'@'localhost';
ERROR 42000: Access denied; you need (at least one of) the SUPER, SET USER privilege(s) for this operation
connection default;
ALTER PROCEDURE p1 DEFINER = 'root'@'localhost';
SELECT definer FROM mysql.proc WHERE name = 'p1';
definer
root@localhost
DROP PROCEDURE p1;
DROP USER 'user1'@'localhost';
Warnings:
Note 4227 Dropped users 'user1'@'localhost' have active connections. Use KILL CONNECTION if they should not be used anymore.
20 changes: 20 additions & 0 deletions mysql-test/main/alter_procedure_definer.test
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
--echo # ALTER PROCEDURE DEFINER
Copy link
Copy Markdown
Member

@gkodinov gkodinov May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We customarily start with a prefix comment like

--echo MDEV-1234 this mdev does so and so


CREATE USER 'user1'@'localhost';
GRANT ALL PRIVILEGES ON test.* TO 'user1'@'localhost';

--connect (con1, localhost, user1, , test)
CREATE PROCEDURE p1() SELECT 1;

ALTER PROCEDURE p1 DEFINER = 'user1'@'localhost';

--error ER_SPECIFIC_ACCESS_DENIED_ERROR
Copy link
Copy Markdown
Member

@gkodinov gkodinov May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please provide a design document someplace: what is supposed to work and what isn't. This should ideally be into the MDEV itself.

ALTER PROCEDURE p1 DEFINER = 'root'@'localhost';

--connection default
ALTER PROCEDURE p1 DEFINER = 'root'@'localhost';

SELECT definer FROM mysql.proc WHERE name = 'p1';

DROP PROCEDURE p1;
DROP USER 'user1'@'localhost';
16 changes: 15 additions & 1 deletion sql/sp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1711,7 +1711,7 @@ Sp_handler::sp_drop_routine(THD *thd,

int
Sp_handler::sp_update_routine(THD *thd, const Database_qualified_name *name,
const st_sp_chistics *chistics) const
const st_sp_chistics *chistics, LEX_USER *definer) const
{
TABLE *table;
int ret;
Expand Down Expand Up @@ -1768,6 +1768,20 @@ Sp_handler::sp_update_routine(THD *thd, const Database_qualified_name *name,
if (chistics->agg_type != DEFAULT_AGGREGATE)
table->field[MYSQL_PROC_FIELD_AGGREGATE]->
store((longlong)chistics->agg_type, TRUE);
if (definer)
{
if (definer->user.str && definer->host.str)
{
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);
Comment on lines +1775 to +1781
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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);

sp_cache_invalidate();
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

}
}
if ((ret= table->file->ha_update_row(table->record[1],table->record[0])) &&
ret != HA_ERR_RECORD_IS_THE_SAME)
ret= SP_WRITE_ROW_FAILED;
Expand Down
2 changes: 1 addition & 1 deletion sql/sp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ class Sp_handler
bool sp_create_routine(THD *thd, const sp_head *sp) const;

int sp_update_routine(THD *thd, const Database_qualified_name *name,
const st_sp_chistics *chistics) const;
const st_sp_chistics *chistics, LEX_USER *definer = nullptr) const;

int sp_drop_routine(THD *thd, const Database_qualified_name *name) const;

Expand Down
9 changes: 8 additions & 1 deletion sql/sql_parse.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6448,14 +6448,21 @@ alter_routine(THD *thd, LEX *lex)
if (check_routine_access(thd, ALTER_PROC_ACL, &lex->spname->m_db,
&lex->spname->m_name, sph, 0))
return 1;
if (lex->definer)
{
if ((!lex_string_eq(&lex->definer->user, thd->security_ctx->priv_user, strlen(thd->security_ctx->priv_user)) ||
Copy link
Copy Markdown
Member

@gkodinov gkodinov May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

!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))
Copy link
Copy Markdown
Member

@gkodinov gkodinov May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never re-use privileges like that. This turns them into roles.

return 1;
}
Comment on lines +6451 to +6457
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;

/*
Note that if you implement the capability of ALTER FUNCTION to
alter the body of the function, this command should be made to
follow the restrictions that log-bin-trust-function-creators=0
already puts on CREATE FUNCTION.
*/
/* 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);
Copy link
Copy Markdown
Member

@gkodinov gkodinov May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

switch (sp_result) {
case SP_OK:
my_ok(thd);
Expand Down
9 changes: 7 additions & 2 deletions sql/sql_yacc.yy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1777,7 +1777,7 @@ rule:

%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
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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


%type <user_auth> opt_auth_str auth_expression auth_token
text_or_password
Expand Down Expand Up @@ -3395,7 +3395,12 @@ sp_name:

sp_a_chistics:
/* Empty */ {}
| sp_a_chistics sp_chistic {}
| sp_a_chistics sp_a_chistic {}
;

sp_a_chistic:
sp_chistic {}
| definer {}
;

sp_c_chistics:
Expand Down