From 72429cad7f5448ad680d8df7b55aa055462d5964 Mon Sep 17 00:00:00 2001 From: Nikita Malyavin Date: Wed, 28 Dec 2022 23:05:46 +0300 Subject: [PATCH] MDEV-30046 wrong row targeted with "insert ... on duplicate" and "replace" When HA_DUPLICATE_POS is not supported, the row to replace was navigated by ha_index_read_idx_map, which uses only hash to navigate. Suchwise, given a hash collision it may choose an incorrect row. handler::position would be correct and very convenient to use here. dup_ref is already set by handler independently of the engine capabilities, when an extra lookup is made (for long unique or something else, for example WITHOUT OVERLAPS) such error will be indicated by file->lookup_errkey != -1. --- mysql-test/main/long_unique_bugs.result | 19 +++++++++++++++++++ mysql-test/main/long_unique_bugs.test | 15 +++++++++++++++ sql/handler.cc | 13 ++++++++----- sql/handler.h | 1 + sql/sql_insert.cc | 23 ++++++++++++++++++++--- 5 files changed, 63 insertions(+), 8 deletions(-) diff --git a/mysql-test/main/long_unique_bugs.result b/mysql-test/main/long_unique_bugs.result index 81482e68417..b01a0dd2256 100644 --- a/mysql-test/main/long_unique_bugs.result +++ b/mysql-test/main/long_unique_bugs.result @@ -674,6 +674,25 @@ SELECT * FROM t1; a b c 3 2 2 DROP TABLE t1; +# MDEV-30046 wrong row targeted with "insert ... on duplicate" and +# "replace", leading to data corruption +create table t (s blob, n int, unique (s)) engine=innodb; +insert into t values ('Hrecvx_0004ln-00',1), ('Hrecvx_0004mm-00',1); +replace into t values ('Hrecvx_0004mm-00',2); +select * from t; +s n +Hrecvx_0004ln-00 1 +Hrecvx_0004mm-00 2 +drop table t; +create table t (s blob, n int, unique (s)) engine=innodb; +insert into t values ('Hrecvx_0004ln-00',1), ('Hrecvx_0004mm-00',1); +insert into t values ('Hrecvx_0004mm-00',2) +on duplicate key update n = values (n); +select * from t; +s n +Hrecvx_0004ln-00 1 +Hrecvx_0004mm-00 2 +drop table t; # # End of 10.5 tests # diff --git a/mysql-test/main/long_unique_bugs.test b/mysql-test/main/long_unique_bugs.test index 2441dfa59e8..3c2ab4d70f5 100644 --- a/mysql-test/main/long_unique_bugs.test +++ b/mysql-test/main/long_unique_bugs.test @@ -655,6 +655,21 @@ REPLACE INTO t1 VALUES (3,2,2); SELECT * FROM t1; DROP TABLE t1; +--echo # MDEV-30046 wrong row targeted with "insert ... on duplicate" and +--echo # "replace", leading to data corruption +--source include/have_innodb.inc +create table t (s blob, n int, unique (s)) engine=innodb; +insert into t values ('Hrecvx_0004ln-00',1), ('Hrecvx_0004mm-00',1); +replace into t values ('Hrecvx_0004mm-00',2); +select * from t; +drop table t; + +create table t (s blob, n int, unique (s)) engine=innodb; +insert into t values ('Hrecvx_0004ln-00',1), ('Hrecvx_0004mm-00',1); +insert into t values ('Hrecvx_0004mm-00',2) + on duplicate key update n = values (n); +select * from t; +drop table t; --echo # --echo # End of 10.5 tests --echo # diff --git a/sql/handler.cc b/sql/handler.cc index 7f5cb343824..e2fe80a260d 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -4569,6 +4569,12 @@ uint handler::get_dup_key(int error) DBUG_RETURN(errkey); } +bool handler::has_dup_ref() const +{ + DBUG_ASSERT(lookup_errkey != (uint)-1 || errkey != (uint)-1); + return ha_table_flags() & HA_DUPLICATE_POS || lookup_errkey != (uint)-1; +} + /** Delete all files with extension from bas_ext(). @@ -6996,11 +7002,8 @@ exit: if (error == HA_ERR_FOUND_DUPP_KEY) { table->file->lookup_errkey= key_no; - if (ha_table_flags() & HA_DUPLICATE_POS) - { - lookup_handler->position(table->record[0]); - memcpy(table->file->dup_ref, lookup_handler->ref, ref_length); - } + lookup_handler->position(table->record[0]); + memcpy(table->file->dup_ref, lookup_handler->ref, ref_length); } restore_record(table, file->lookup_buffer); table->restore_blob_values(blob_storage); diff --git a/sql/handler.h b/sql/handler.h index 0b0ffe6be27..f858553c7b7 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -3464,6 +3464,7 @@ public: virtual void print_error(int error, myf errflag); virtual bool get_error_message(int error, String *buf); uint get_dup_key(int error); + bool has_dup_ref() const; /** Retrieves the names of the table and the key for which there was a duplicate entry in the case of HA_ERR_FOREIGN_DUPLICATE_KEY. diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index b3314e075f0..1e0a6a75338 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1884,11 +1884,28 @@ int write_record(THD *thd, TABLE *table, COPY_INFO *info, select_result *sink) if (info->handle_duplicates == DUP_REPLACE && table->next_number_field && key_nr == table->s->next_number_index && insert_id_for_cur_row > 0) goto err; - if (table->file->ha_table_flags() & HA_DUPLICATE_POS) + if (table->file->has_dup_ref()) { + /* + If engine doesn't support HA_DUPLICATE_POS, the handler may init to + INDEX, but dup_ref could also be set by lookup_handled (and then, + lookup_errkey is set, f.ex. long unique duplicate). + + In such case, handler would stay uninitialized, so do it here. + */ + bool init_lookup_handler= table->file->lookup_errkey != (uint)-1 && + table->file->inited == handler::NONE; + if (init_lookup_handler && table->file->ha_rnd_init_with_error(false)) + goto err; + DBUG_ASSERT(table->file->inited == handler::RND); - if (table->file->ha_rnd_pos(table->record[1],table->file->dup_ref)) - goto err; + int rnd_pos_err= table->file->ha_rnd_pos(table->record[1], + table->file->dup_ref); + + if (init_lookup_handler) + table->file->ha_rnd_end(); + if (rnd_pos_err) + goto err; } else {