Description:
clone_server.cc:Server::parse_comand_buffer assumes that COM_EXIT cannot fail, and always returns zero:
int Server::parse_command_buffer(uchar command, uchar *com_buf, size_t com_len,
bool &done) {
int err = 0;
...
switch (com) {
...
case COM_EXIT:
if (m_storage_initialized) {
hton_clone_end(get_thd(), get_storage_vector(), m_tasks, 0);
m_storage_initialized = false;
}
done = true;
log_error(get_thd(), false, err, "COM_EXIT: Storage End");
break;
...
return (err);
}
This zero propagates up the call stack until is acted upon in sql_admin.cc:Sql_cmd_clone::execute_server:
auto err = m_clone->clone_remote_server(thd, sock);
if (err == 0) {
my_ok(thd);
}
...
if (err != 0) {
...
But, COM_EXIT implementation can set an error under some conditions, in innodb_clone_end:
my_error(ER_QUERY_INTERRUPTED, MYF(0));
my_error(ER_QUERY_INTERRUPTED, MYF(0));
my_error(ER_CLONE_DDL_IN_PROGRESS, MYF(0));
If any of these happen, Sql_cmd_clone::execute_server will execute the wrong one of success/failure branches, at the very least following up the my_error call with an my_ok call (with a corresponding debug build assert), and not doing any of the if (err != 0) diagnostic actions.
How to repeat:
Code analysis above
Suggested fix:
Propagate return code from innodb_clone_end all the way to Sql_cmd_clone::execute_server
Description: clone_server.cc:Server::parse_comand_buffer assumes that COM_EXIT cannot fail, and always returns zero: int Server::parse_command_buffer(uchar command, uchar *com_buf, size_t com_len, bool &done) { int err = 0; ... switch (com) { ... case COM_EXIT: if (m_storage_initialized) { hton_clone_end(get_thd(), get_storage_vector(), m_tasks, 0); m_storage_initialized = false; } done = true; log_error(get_thd(), false, err, "COM_EXIT: Storage End"); break; ... return (err); } This zero propagates up the call stack until is acted upon in sql_admin.cc:Sql_cmd_clone::execute_server: auto err = m_clone->clone_remote_server(thd, sock); if (err == 0) { my_ok(thd); } ... if (err != 0) { ... But, COM_EXIT implementation can set an error under some conditions, in innodb_clone_end: my_error(ER_QUERY_INTERRUPTED, MYF(0)); my_error(ER_QUERY_INTERRUPTED, MYF(0)); my_error(ER_CLONE_DDL_IN_PROGRESS, MYF(0)); If any of these happen, Sql_cmd_clone::execute_server will execute the wrong one of success/failure branches, at the very least following up the my_error call with an my_ok call (with a corresponding debug build assert), and not doing any of the if (err != 0) diagnostic actions. How to repeat: Code analysis above Suggested fix: Propagate return code from innodb_clone_end all the way to Sql_cmd_clone::execute_server