MDEV-23299: The udf_init() function cause server crash. #1643

zbdba · 2020-07-27T12:10:47Z

No description provided.

an3l · 2020-07-27T18:45:00Z

Hi @zbdba,
Thanks for your contribution.

Similar to other open source projects, the MariaDB Foundation needs to have shared ownership of all code that is included in the MariaDB distribution. The easiest way to achieve this is by submitting your code under the BSD-new license.

The other alternative is to sign the code contribution agreement which can be found here: https://mariadb.com/kb/en/mariadb/mca/

Please indicate in a comment below, or update your PR comment, that you are contributing your new code of the whole pull request, including one or several files that are either new files or modified ones, under the BSD-new license or that you have filled out the contribution agreement and sent it.

Thanks,
Anel

grooverdan

Rather than testing the value on retrieval, II recommend adding CHECK constrains so that non-empty values can't be inserted.

For completeness on checks ret can be 0, 1, 2, 4 (corresponding to STRING_RESULT=0, REAL_RESULT, INT_RESULT, DECIMAL_RESULT (not ROW_RESULT).

Include checks in definition in scripts/mysql_system_tables.sql.
include removal of rows the violate checks, and alter table to add checks in scripts/mysql_system_tables_fix.sql.
include some tests of manual inserting into the func table in mysql-test/t/create_drop_udf.test

grooverdan · 2020-07-27T23:39:55Z

sql/sql_udf.cc

@@ -206,6 +206,15 @@ void udf_init()
    DBUG_PRINT("info",("init udf record"));
    LEX_CSTRING name;
    name.str=get_field(&mem, table->field[0]);
+
+    // Check the name.str is NULL or not.
+    if (name.str == nullptr)


The name in the mysql.func table is declared NOT NULL so this isn't the case that is hit when insert into mysql.func(ret) values(1); is performed.

name will be an empty string (or easier name.length=0)

grooverdan · 2020-07-27T23:41:36Z

sql/sql_udf.cc

+    if (name.str == nullptr)
+    {
+      sql_print_error("Invalid row in mysql.func table for function '%.64s'",
+                      name.str);


If the name.str ls nullptr it can't be used in sql_print_error. If its empty "" always then there's not much point in displaying it.

zbdba · 2020-07-28T02:26:48Z

hi @an3l
I have signed the code contribution agreement and send email to.

zbdba · 2020-07-28T02:38:20Z

hi @grooverdan

Your plan sounds better,more completely. and who can make sure this plan. I'm glad to contribute it.

grooverdan · 2020-07-28T02:39:17Z

out of curiosity @zbdba, what prompted you to test the mysql.func table in this manner?

zbdba · 2020-07-28T02:45:02Z

@grooverdan , I have the same question with you, this made by our customer.

grooverdan · 2020-07-28T02:52:12Z

@zbdba I'd really like a contribution from you on the revised plan that I wrote.

zbdba · 2020-07-28T02:55:30Z

@grooverdan thanks, and who can make sure this? @an3l

grooverdan · 2020-07-28T02:56:59Z

Either me or @an3l will merge a completed implementation.

zbdba · 2020-07-28T02:59:47Z

@grooverdan ok, wait my pull request.

an3l · 2020-07-28T07:29:26Z

@zbdba if possible, please don't close the current PR to open the new one, you can do all your changes in this PR, if you need help let me know.
Also, let me know if you need help to create the test case.

zbdba · 2020-07-28T07:31:17Z

@an3l ok, thanks, I will do some changes in this PR.

zbdba · 2020-07-29T14:53:02Z

@grooverdan @an3l I have changed my PR, please help review my code.
thanks.

an3l · 2020-07-29T21:42:02Z

mysql-test/main/create_drop_udf.test

@@ -35,3 +35,19 @@ SELECT metaphon('mariadb');

 DROP FUNCTION metaphon;
 DROP FUNCTION IF EXISTS metaphon;
+
+--echo #
+--echo # MDEV-23299


Test case is missing description of MDEV, usually should be as in jira.

an3l · 2020-07-29T21:43:02Z

mysql-test/main/create_drop_udf.test

+--echo #
+--echo # MDEV-23299
+--echo #
+INSERT into mysql.func(name, ret, dl) values('example', 0, 'example.so');


You are missing the test case described in jira (although is covered by other cases,but one more example would describing the formulation of the problem would be great)

INSERT into mysql.func(ret) values(0);

an3l · 2020-07-29T22:15:27Z

scripts/mysql_system_tables_fix.sql

@@ -113,6 +113,14 @@ ALTER TABLE columns_priv

 ALTER TABLE func add type enum ('function','aggregate') COLLATE utf8_general_ci NOT NULL;

+#


I think this complete change is not needed.

an3l · 2020-07-29T22:19:02Z

scripts/mysql_system_tables.sql

@@ -104,7 +104,7 @@ set @had_user_table= @@warning_count != 0;

 CREATE TABLE IF NOT EXISTS roles_mapping ( Host char(60) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Role char(80) binary DEFAULT '' NOT NULL, Admin_option enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, UNIQUE (Host, User, Role)) engine=Aria transactional=1 CHARACTER SET utf8 COLLATE utf8_bin comment='Granted roles';

-CREATE TABLE IF NOT EXISTS func (  name char(64) binary DEFAULT '' NOT NULL, ret tinyint(1) DEFAULT '0' NOT NULL, dl char(128) DEFAULT '' NOT NULL, type enum ('function','aggregate') COLLATE utf8_general_ci NOT NULL, PRIMARY KEY (name) ) engine=Aria transactional=1 CHARACTER SET utf8 COLLATE utf8_bin   comment='User defined functions';
+CREATE TABLE IF NOT EXISTS func (  name char(64) COLLATE utf8_bin NOT NULL CHECK (name <> ''), ret tinyint(1) DEFAULT '0' NOT NULL CHECK (ret < 5 and ret >= 0 and ret <> 3), dl char(128) COLLATE utf8_bin NOT NULL CHECK (dl <> ''), type enum ('function','aggregate') COLLATE utf8_general_ci NOT NULL, PRIMARY KEY (name) ) engine=Aria transactional=1 CHARACTER SET utf8 COLLATE utf8_bin   comment='User defined functions';


Why exactly to change collation ?
And what is reason for setting the check of the ret? Also which logic you used for this set and why?

It was me that suggested the ret constraints above. The values correspond the the value types that UDF functions can return. For completeness this should be in the commit message.

Something like
"return values (ret) can be STRING_RESULT(0), REAL_RESULT(1), INT_RESULT(2), and DECIMAL_RESULT(4), but not ROW_RESULT(3) or TIME_RESULT(5)".

The commit message should also have a title that begins with 'MDEV-23299:......'

I just checked and udf can't return TIME_RESULT(5).

What do you think about ret <>3 AND ret BETWEEN 0 AND 4)? read-ability is the criteria.

an3l · 2020-07-29T22:22:08Z

sql/sql_udf.cc

@@ -206,15 +206,6 @@ void udf_init()
    DBUG_PRINT("info",("init udf record"));
    LEX_CSTRING name;
    name.str=get_field(&mem, table->field[0]);
-


Here we have older commit which we don't need.
You will have to do git rebase and push new changes.

an3l · 2020-07-29T22:24:46Z

mysql-test/main/create_drop_udf.test

+--echo #
+INSERT into mysql.func(name, ret, dl) values('example', 0, 'example.so');
+
+--error ER_CONSTRAINT_FAILED 


Here and in second error you have whitespace at the end.

grooverdan

pretty close. Thanks for the quick implemenation

grooverdan · 2020-07-30T01:06:39Z

mysql-test/main/create_drop_udf.test

+--error ER_CONSTRAINT_FAILED 
+INSERT into mysql.func(name, ret, dl) values('example', 0, '');
+
+DELETE from mysql.func;


constrain the DELETE to the row that was successfully inserted.

grooverdan · 2020-07-30T01:22:42Z

scripts/mysql_system_tables.sql

@@ -104,7 +104,7 @@ set @had_user_table= @@warning_count != 0;

 CREATE TABLE IF NOT EXISTS roles_mapping ( Host char(60) binary DEFAULT '' NOT NULL, User char(80) binary DEFAULT '' NOT NULL, Role char(80) binary DEFAULT '' NOT NULL, Admin_option enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, UNIQUE (Host, User, Role)) engine=Aria transactional=1 CHARACTER SET utf8 COLLATE utf8_bin comment='Granted roles';

-CREATE TABLE IF NOT EXISTS func (  name char(64) binary DEFAULT '' NOT NULL, ret tinyint(1) DEFAULT '0' NOT NULL, dl char(128) DEFAULT '' NOT NULL, type enum ('function','aggregate') COLLATE utf8_general_ci NOT NULL, PRIMARY KEY (name) ) engine=Aria transactional=1 CHARACTER SET utf8 COLLATE utf8_bin   comment='User defined functions';
+CREATE TABLE IF NOT EXISTS func (  name char(64) COLLATE utf8_bin NOT NULL CHECK (name <> ''), ret tinyint(1) DEFAULT '0' NOT NULL CHECK (ret < 5 and ret >= 0 and ret <> 3), dl char(128) COLLATE utf8_bin NOT NULL CHECK (dl <> ''), type enum ('function','aggregate') COLLATE utf8_general_ci NOT NULL, PRIMARY KEY (name) ) engine=Aria transactional=1 CHARACTER SET utf8 COLLATE utf8_bin   comment='User defined functions';


It was me that suggested the ret constraints above. The values correspond the the value types that UDF functions can return. For completeness this should be in the commit message.

Something like
"return values (ret) can be STRING_RESULT(0), REAL_RESULT(1), INT_RESULT(2), and DECIMAL_RESULT(4), but not ROW_RESULT(3) or TIME_RESULT(5)".

The commit message should also have a title that begins with 'MDEV-23299:......'

I just checked and udf can't return TIME_RESULT(5).

What do you think about ret <>3 AND ret BETWEEN 0 AND 4)? read-ability is the criteria.

grooverdan · 2020-07-30T01:28:04Z

scripts/mysql_system_tables_fix.sql

+#
+
+ALTER TABLE func MODIFY name char(64) COLLATE utf8_bin NOT NULL CHECK (name <> '');
+ALTER TABLE func MODIFY ret tinyint(1) DEFAULT '0' NOT NULL CHECK (ret < 5 and ret >= 0 and ret <> 3);


ALTER TABLE can handle several modifications at with comma separated modifications. Please use this form, sql over multiple lines with indents is acceptable. Check needs to match other definition.

For alter to succeed, the contraints need to match for all elements in the table.
DELETE FROM func WHERE name='' OR ret=3 OR NOT ret BETWEEN 0 AND 4 OR dl=''

grooverdan · 2020-07-30T01:29:00Z

scripts/mysql_system_tables_fix.sql

+# Add CHECK for 'name','ret','dl' field.
+#
+
+ALTER TABLE func MODIFY name char(64) COLLATE utf8_bin NOT NULL CHECK (name <> '');


remove explicit collate.

grooverdan · 2020-07-30T01:51:26Z

scripts/mysql_system_tables_fix.sql

+
+ALTER TABLE func MODIFY name char(64) COLLATE utf8_bin NOT NULL CHECK (name <> '');
+ALTER TABLE func MODIFY ret tinyint(1) DEFAULT '0' NOT NULL CHECK (ret < 5 and ret >= 0 and ret <> 3);
+ALTER TABLE func MODIFY dl char(128) COLLATE utf8_bin NOT NULL CHECK (dl <> '');


no collate here either.

zbdba · 2020-07-30T02:09:46Z

@an3l @grooverdan thanks for review my code, I will change the problem code, and wait for you confirm whether ret should have CHECK or not.I think maybe the ret have another result in the future？

grooverdan · 2020-07-30T02:22:51Z

confirm whether ret should have CHECK or not.I think maybe the ret have another result in the future？

I'm happy either way. If UDF gets extended it will quickly hit this condition when they add a test case for it.

zbdba · 2020-07-30T06:25:36Z

@grooverdan @an3l I have change my PR again, please help review my code, thanks.

udf_init() can crash when the name field is empty. We correct this by applying CHECK column constraints on columns in the mysql.func tables. The constrains are as follows: * name cannot be an empty string; * ret, the return type can be; * STRING_RESULT(0), * REAL_RESULT(1), * INT_RESULT(2), * and DECIMAL_RESULT(4); * but not ROW_RESULT(3) or TIME_RESULT(5). * dl, cannot be empty string Upgrades ensure that any invalid functions are deleted. closes #1643

vuvova · 2020-08-01T07:44:27Z

I don't see it's needed. The root user can drop constraints and insert invalid data, it can drop the whole table, or remove essential columns. Or simply install a malicious UDF.

It would be slightly safer to make the server not to crash on empty UDF names. But at the end of the day, a root user with all privileges in the server can cause any amount of damage and we cannot prevent it.

zbdba · 2020-08-02T14:42:59Z

@vuvova This situation is made by our customers. I think there is at least one way to protect against server crash,add CHECK for table or fix the problem code which make server crash.

zbdba · 2020-08-04T14:31:48Z

@grooverdan @an3l What status the PR? will merge into or just close with do nothing?

grooverdan · 2020-08-06T08:49:10Z

Hi @zbdba. We haven't forgotten. Its a very busy pre-release so I hope to get a resolution after some current pressure is off the development team. I appreciate your patience and rapid delivery of working code and responding to our reviews.

grooverdan · 2020-08-12T02:28:51Z

It appears a number of key development staff I wanted to catch up with an talk about this are taking leave for a few weeks. I'll catch them when they get back. Appreciate your patience.

zbdba · 2020-08-14T08:08:58Z

@grooverdan ok, thanks.

LinuxJedi · 2023-08-03T15:16:58Z

Hi @zbdba,

It looks like we dropped the ball on this one, for which I apologise. I've updated with a rebase to re-trigger the CI system and will review it for you soon.

LinuxJedi · 2023-08-03T15:47:47Z

Hi @zbdba,

Thank you again for your contribution. Unfortunately, upon reviewing this, I found that this has since been fixed in commit 4f63b6c. I am therefore going to close this PR at this time.

Please don't hesitate to reopen this if you think it is in error, or contact me if you have any questions.

an3l added this to the 10.5 milestone Jul 27, 2020

grooverdan requested changes Jul 27, 2020

View reviewed changes

an3l added the license-mca Contributed under the MCA label Jul 28, 2020

cvicentiu assigned grooverdan Jul 28, 2020

an3l requested changes Jul 29, 2020

View reviewed changes

grooverdan requested changes Jul 30, 2020

View reviewed changes

zbdba force-pushed the MDEV-23299 branch from 412d837 to e543ab6 Compare July 30, 2020 06:24

zbdba added 2 commits August 3, 2023 16:15

Add CHECK for mysql.func to fix empty string value cause server crash.

b48d620

fix some problem code.

bcc8b57

LinuxJedi force-pushed the MDEV-23299 branch from e543ab6 to bcc8b57 Compare August 3, 2023 15:15

LinuxJedi closed this Aug 3, 2023

		@@ -113,6 +113,14 @@ ALTER TABLE columns_priv

		ALTER TABLE func add type enum ('function','aggregate') COLLATE utf8_general_ci NOT NULL;

		#

MDEV-23299: The udf_init() function cause server crash. #1643

MDEV-23299: The udf_init() function cause server crash. #1643

Conversation

zbdba commented Jul 27, 2020

an3l commented Jul 27, 2020

grooverdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbdba commented Jul 28, 2020

zbdba commented Jul 28, 2020

grooverdan commented Jul 28, 2020

zbdba commented Jul 28, 2020

grooverdan commented Jul 28, 2020

zbdba commented Jul 28, 2020

grooverdan commented Jul 28, 2020

zbdba commented Jul 28, 2020

an3l commented Jul 28, 2020

zbdba commented Jul 28, 2020

zbdba commented Jul 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grooverdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbdba commented Jul 30, 2020

grooverdan commented Jul 30, 2020

zbdba commented Jul 30, 2020

vuvova commented Aug 1, 2020

zbdba commented Aug 2, 2020

zbdba commented Aug 4, 2020

grooverdan commented Aug 6, 2020

grooverdan commented Aug 12, 2020

zbdba commented Aug 14, 2020

LinuxJedi commented Aug 3, 2023

LinuxJedi commented Aug 3, 2023