From 5b665f0069bada02ccd0be6bcbbc563d343667cd Mon Sep 17 00:00:00 2001 From: Brandon Nesterenko Date: Mon, 6 Apr 2026 12:53:25 -0600 Subject: [PATCH] MDEV-38830: SIGSEGV and UBSAN null-pointer-use in TABLE::evaluate_update_default_function on UPDATE MDEV-38716 (fa36b269f13) fixes were incomplete. It still allowed a table's default_fields to be left un-restored after an ALTER table finished its copy_data_between_fields(). This patch actually matches the original conception of MDEV-38716. It was later changed after finding a test failure in maria.alter, where I thought the patch broke that test. But actually, maria.alter itself relies on somewhat broken/inconsistent server behavior. It is the MDEV-19055 extension of the test which broke. To summarize the broken part of the test, first, it creates a temporary table t2, adds a new column of type DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, and adds a constraint check on that new type: CREATE TEMPORARY TABLE t2 (a TIME) ENGINE=Aria; ALTER TABLE t2 ADD b DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP; ALTER TABLE t2 ADD CHECK (b = 4); The next part results in inconsistent behavior: INSERT IGNORE INTO t2 () VALUES (),(),(),(); Prior to this patch, this would create 4 new rows, each with a zeroed out timestamp for `b`. This is due to a the default_field not resetting after the ALTER TABLE, thus the DEFAULT CURRENT_TIMESTAMP clause is lost after the ALTER TABLE ADD CHECK. With this patch, because the default_field is restored after the ALTER, there is no affect of the INSERT IGNORE INTO t2. I.e., t2 is empty after this insert. This change in results further changes how an ALTER TABLE behaves later in the test: SET SQL_MODE= 'STRICT_ALL_TABLES'; SELECT count(a),sum(a) FROM t2; --error ER_TRUNCATED_WRONG_VALUE ALTER TABLE t2 CHANGE IF EXISTS d c INT; Without this patch, there are rows in t2, and so this ALTER TABLE results in an error. With this patch, t2 is empty, and therefore there is no data that was truncated to warn about. It is generally bad test practice to have a table with no rows, as it has historically masked other bugs. So we add back a couple rows into the table to ensure it hits additional logic. However, because the bug has been fixed, invalid rows are not able to be inserted into the table and thereby the ER_TRUNCATED_WRONG_VALUE error is still not thrown. So that error is removed. Reviewed-by: Monty Signed-off-by: Brandon Nesterenko --- mysql-test/main/temp_table.result | 11 +++++ mysql-test/main/temp_table.test | 12 +++++ mysql-test/suite/maria/alter.result | 15 ++++--- mysql-test/suite/maria/alter.test | 5 ++- sql/sql_table.cc | 69 +++++++++++++++++++---------- 5 files changed, 79 insertions(+), 33 deletions(-) diff --git a/mysql-test/main/temp_table.result b/mysql-test/main/temp_table.result index 75f9cd1eed6..a4f01fbdad8 100644 --- a/mysql-test/main/temp_table.result +++ b/mysql-test/main/temp_table.result @@ -746,3 +746,14 @@ SELECT * FROM tmp_38716_2 where b=20; a b c NULL 20 21 DROP TABLE tmp_38716_2; +# +# MDEV-38830 +# +CREATE TEMPORARY TABLE tmp_38830 (c INT KEY); +INSERT INTO tmp_38830 VALUES (1); +ALTER TABLE tmp_38830 ADD COLUMN (d TIMESTAMP ON UPDATE CURRENT_TIMESTAMP); +UPDATE tmp_38830 SET c=c+1; +select * from tmp_38830; +c d +2 2001-01-01 10:20:30 +DROP TEMPORARY TABLE tmp_38830; diff --git a/mysql-test/main/temp_table.test b/mysql-test/main/temp_table.test index ea6201c45ab..fd4a93fbbfa 100644 --- a/mysql-test/main/temp_table.test +++ b/mysql-test/main/temp_table.test @@ -810,3 +810,15 @@ INSERT INTO tmp_38716_2 (b) VALUES (20); SELECT * FROM tmp_38716_2 where b=20; DROP TABLE tmp_38716_2; + + +--echo # +--echo # MDEV-38830 +--echo # +CREATE TEMPORARY TABLE tmp_38830 (c INT KEY); +INSERT INTO tmp_38830 VALUES (1); +ALTER TABLE tmp_38830 ADD COLUMN (d TIMESTAMP ON UPDATE CURRENT_TIMESTAMP); +UPDATE tmp_38830 SET c=c+1; +select * from tmp_38830; + +DROP TEMPORARY TABLE tmp_38830; diff --git a/mysql-test/suite/maria/alter.result b/mysql-test/suite/maria/alter.result index 36d7ae9c302..656c2779ffb 100644 --- a/mysql-test/suite/maria/alter.result +++ b/mysql-test/suite/maria/alter.result @@ -53,12 +53,14 @@ DROP TABLE t1, t2; CREATE OR REPLACE TABLE t1 (x INT) ENGINE=Aria; CREATE TEMPORARY TABLE t2 (a TIME) ENGINE=Aria; ALTER TABLE t2 ADD b DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP; -ALTER TABLE t2 ADD CHECK (b = 4); +ALTER TABLE t2 ADD CHECK (b <= "2001-01-01"); +INSERT INTO t2 values (1,"2000-01-01"); +INSERT INTO t2 values (2,"2000-02-02"); INSERT IGNORE INTO t2 () VALUES (),(),(),(); ALTER IGNORE TABLE t2 ADD c INT; SELECT count(a),sum(a) FROM t2; count(a) sum(a) -0 NULL +2 3 DELETE FROM t2 ORDER BY c LIMIT 1; INSERT IGNORE INTO t2 SELECT * FROM t2; OPTIMIZE TABLE t2; @@ -66,23 +68,22 @@ Table Op Msg_type Msg_text test.t2 optimize status OK SELECT count(a),sum(a) FROM t2; count(a) sum(a) -0 NULL +2 4 INSERT IGNORE INTO t2 SELECT * FROM t2; SET SQL_MODE= 'STRICT_ALL_TABLES'; SELECT count(a),sum(a) FROM t2; count(a) sum(a) -0 NULL +4 8 ALTER TABLE t2 CHANGE IF EXISTS d c INT; -ERROR 22007: Truncated incorrect datetime value: '4' SELECT count(a),sum(a) FROM t2; count(a) sum(a) -0 NULL +4 8 ALTER IGNORE TABLE t2 ADD IF NOT EXISTS e BIT; ALTER TABLE t1 MODIFY IF EXISTS xx INT; INSERT IGNORE INTO t2 () VALUES (),(),(),(); SELECT count(a),sum(a) FROM t2; count(a) sum(a) -0 NULL +4 8 check table t1; Table Op Msg_type Msg_text test.t1 check status OK diff --git a/mysql-test/suite/maria/alter.test b/mysql-test/suite/maria/alter.test index a68b5f2e0d7..2b0c8dd59c2 100644 --- a/mysql-test/suite/maria/alter.test +++ b/mysql-test/suite/maria/alter.test @@ -53,7 +53,9 @@ DROP TABLE t1, t2; CREATE OR REPLACE TABLE t1 (x INT) ENGINE=Aria; CREATE TEMPORARY TABLE t2 (a TIME) ENGINE=Aria; ALTER TABLE t2 ADD b DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP; -ALTER TABLE t2 ADD CHECK (b = 4); +ALTER TABLE t2 ADD CHECK (b <= "2001-01-01"); +INSERT INTO t2 values (1,"2000-01-01"); +INSERT INTO t2 values (2,"2000-02-02"); INSERT IGNORE INTO t2 () VALUES (),(),(),(); ALTER IGNORE TABLE t2 ADD c INT; SELECT count(a),sum(a) FROM t2; @@ -64,7 +66,6 @@ SELECT count(a),sum(a) FROM t2; INSERT IGNORE INTO t2 SELECT * FROM t2; SET SQL_MODE= 'STRICT_ALL_TABLES'; SELECT count(a),sum(a) FROM t2; ---error ER_TRUNCATED_WRONG_VALUE ALTER TABLE t2 CHANGE IF EXISTS d c INT; SELECT count(a),sum(a) FROM t2; ALTER IGNORE TABLE t2 ADD IF NOT EXISTS e BIT; diff --git a/sql/sql_table.cc b/sql/sql_table.cc index c0795cb2ab5..310d5e9784f 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -12776,16 +12776,26 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *to, } /* - Mark the end of the default field list. Note that this is a temporary - state; fields with ON UPDATE defaults will be added back to this list - after the data copy is complete. + Temporarily finalize the state of the default fields list. At this point + (i.e. during the copy of the values), only newly added fields with default + values should exist in the list. dfield_ptr is effectively a tail pointer + into to->default_field. If dfield_ptr (tail) and to->default_field (head) + point to the same point, it means the list is empty and there were no newly + added fields with default values. In such case, nullify to->default_field + (dfield_ptr remains pointing to the old list, as it will be used to restore + to->default_field later). Otherwise, if dfield_ptr (tail) is after the + head, then we nullify its current position to mark the end of the list. + + Note that this is a temporary state; new/existing fields with either ON + UPDATE defaults and existing fields with default values will be added back + to this list after the data copy is complete. */ if (dfield_ptr) { if (dfield_ptr == to->default_field) - to->default_field= NULL; // No default fields left + to->default_field= NULL; else - *dfield_ptr= NULL; // Mark end of default field pointers + *dfield_ptr= NULL; } if (order) @@ -13073,16 +13083,14 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *to, for new fields added to the table (i.e those which should get new default values). Now that the data copy is done, the to->default_field list needs to be restored to include all fields that have default values to fill. - - First, if to->default_field is NULL, it means it was nullified to prevent - trying to update any default values during the copy process because there - were no new fields added with default values to fill in. The list needs to - be restored to the original location so future records in this session can - get default values. - - Then, because to->default_field was reset and now only considers new - fields, it is missing fields that have ON UPDATE clauses (i.e. TIMESTAMP - and DATETIME). So add those back in. + Either: + 1) to->default_field is NULL, in which case either dfield_ptr is also + NULL or the pre-alter table has fields with default values and the + alter doesn't add any new fields with default values. In either case, + simply restore the original default_field list. + 2) Otherwise, to->default_field has been modified in-place and needs to + be manually restored. I.e., it is missing fields that have ON UPDATE + clauses (i.e. TIMESTAMP and DATETIME). So add those back in. Notes: * The code is similar to the dfield_ptr modification earlier in the @@ -13091,26 +13099,39 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *to, data, which requires the table's default_fields to be set up */ if (!to->default_field) + { to->default_field= dfield_ptr; -#ifndef DBUG_OFF + } else { /* - If to->default_field exists, dfield_ptr should point to the NULL list - terminator in the default_field list. + to->default_field was modified in-place, so we need to check to manually + add back missing fields. */ - DBUG_ASSERT(dfield_ptr && !*dfield_ptr); - } -#endif - if (dfield_ptr) - { it.rewind(); for (Field **ptr= to->field; *ptr; ptr++) { def= it++; - if (def->field && def->field->has_update_default_function()) + + /* + New fields with a default_value already exist in to->default_field. + Here add the rest back: existing fields with any default, and new + fields with only ON UPDATE. + */ + const bool is_new_field= def->field == nullptr; + const bool has_default= (*ptr)->default_value; + const bool has_update_default= (*ptr)->has_update_default_function(); + const bool should_append= is_new_field + ? (!has_default && has_update_default) + : (has_default || has_update_default); + if (should_append) *(dfield_ptr++)= *ptr; } + + /* + Finalize the default_field list. See the comment earlier in this function + where this same pattern is followed for an explanation. + */ if (dfield_ptr == to->default_field) to->default_field= NULL; // No default fields else