Sujatha, Kristian, howdy. Sorry, I managed to sent my view without actually cc-ing Kristian, as claimed. X-From-Line: nobody Wed Nov 27 19:27:51 2019 From: Andrei Elkin <andrei.elkin@mariadb.com> To: sujatha <sujatha.sivakumar@mariadb.com> Cc: <commits@mariadb.org>, <maria-developers@lists.launchpad.net> Subject: Re: f72427f463d: MDEV-20923:UBSAN: member access within address Let me try that again now. Kristian, the question was about `xid_count_per_binlog' struct comment -- .. struct xid_count_per_binlog : public ilink { char *binlog_name; uint binlog_name_len; ulong binlog_id; /* Total prepared XIDs and pending checkpoint requests in this binlog. */ long xid_count; /* For linking in requests to the binlog background thread. */ xid_count_per_binlog *next_in_queue; xid_count_per_binlog(); /* Give link error if constructor used. */ .. -----------------------------^ }; Now I am guessing a constructor was attempted to fail out.. Was that related to de-POD-ing of the object ('cos of the constructor)? It's pretty obscure. We would appreciate your explanation. The former review mail is quoted (except one more [@]-marked view note, Sujatha) further. Cheers, Andrei ------------------------------------------------------------------------------- Sujatha, hello. The patch looks quite good. I have two suggestions though. The first is to match the destructor's free() with actual allocation (of what is to be freed) in the new parameterized constructor. And 2nd-ly, I suggest to leave the explicit zero argument constructor intact. Kristian is cc-d to help us to decide. My comments are below. Cheers, Andrei sujatha <sujatha.sivakumar@mariadb.com> writes:

revision-id: f72427f463d316a54ebf87c2e84c73947e3c5fe4 (mariadb-10.1.43-5-gf72427f463d) parent(s): 13db50fc03e7312e6c01b06c7e4af69f69ba5382 author: Sujatha committer: Sujatha timestamp: 2019-11-12 16:11:16 +0530 message:

MDEV-20923:UBSAN: member access within address … which does not point to an object of type 'xid_count_per_binlog'

Problem: ------- Accessing a member within 'xid_count_per_binlog' structure results in following error when 'UBSAN' is enabled.

member access within address 0xXXX which does not point to an object of type 'xid_count_per_binlog'

Analysis: --------- The problem appears to be that no constructor for 'xid_count_per_binlog' is being called, and thus the vtable will not be initialized.

Fix: --- Defined a parameterized constructor for 'xid_count_per_binlog' class.

--- sql/log.cc | 28 ++++++++++++++-------------- sql/log.h | 9 ++++++++- 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/sql/log.cc b/sql/log.cc index acf1f8f8a9c..2b8b67febef 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -3216,7 +3216,7 @@ void MYSQL_BIN_LOG::cleanup() DBUG_ASSERT(!binlog_xid_count_list.head()); WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::cleanup(): Removing xid_list_entry " "for %s (%lu)", b); - my_free(b); + delete b; }

mysql_mutex_destroy(&LOCK_log); @@ -3580,17 +3580,17 @@ bool MYSQL_BIN_LOG::open(const char *log_name, */ uint off= dirname_length(log_file_name); uint len= strlen(log_file_name) - off; - char *entry_mem, *name_mem; - if (!(new_xid_list_entry = (xid_count_per_binlog *) - my_multi_malloc(MYF(MY_WME), - &entry_mem, sizeof(xid_count_per_binlog), - &name_mem, len, - NULL))) + char *name_mem;

+ name_mem= (char *) my_malloc(len, MYF(MY_ZEROFILL)); + if (!name_mem) goto err; memcpy(name_mem, log_file_name+off, len);

That is both my_malloc and memcpy to go into the new constructor to match [*] (find the label below).

- new_xid_list_entry->binlog_name= name_mem; - new_xid_list_entry->binlog_name_len= len; - new_xid_list_entry->xid_count= 0; + new_xid_list_entry= new xid_count_per_binlog(name_mem, (int)len); + if (!new_xid_list_entry) + { + my_free(name_mem); + goto err; + }

/* Find the name for the Initial binlog checkpoint. @@ -3711,7 +3711,7 @@ bool MYSQL_BIN_LOG::open(const char *log_name, { WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::open(): Removing xid_list_entry for " "%s (%lu)", b); - my_free(binlog_xid_count_list.get()); + delete binlog_xid_count_list.get(); } mysql_cond_broadcast(&COND_xid_list); WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::open(): Adding new xid_list_entry for " @@ -3758,7 +3758,7 @@ Turning logging off for the whole duration of the MySQL server process. \ To turn it on again: fix the cause, \ shutdown the MySQL server and restart it.", name, errno);

if (new_xid_list_entry) - my_free(new_xid_list_entry); + delete new_xid_list_entry; [@] With `delete` we don't need the non-null check: `if()` to remove.

if (file >= 0) mysql_file_close(file, MYF(0)); close(LOG_CLOSE_INDEX); @@ -4252,7 +4252,7 @@ bool MYSQL_BIN_LOG::reset_logs(THD *thd, bool create_new_log, DBUG_ASSERT(b->xid_count == 0); WSREP_XID_LIST_ENTRY("MYSQL_BIN_LOG::reset_logs(): Removing " "xid_list_entry for %s (%lu)", b); - my_free(binlog_xid_count_list.get()); + delete binlog_xid_count_list.get(); } mysql_cond_broadcast(&COND_xid_list); reset_master_pending--; @@ -9736,7 +9736,7 @@ TC_LOG_BINLOG::mark_xid_done(ulong binlog_id, bool write_checkpoint) break; WSREP_XID_LIST_ENTRY("TC_LOG_BINLOG::mark_xid_done(): Removing " "xid_list_entry for %s (%lu)", b); - my_free(binlog_xid_count_list.get()); + delete binlog_xid_count_list.get(); }

mysql_mutex_unlock(&LOCK_xid_list); diff --git a/sql/log.h b/sql/log.h index b4c9b24a3a9..30a55e577a4 100644 --- a/sql/log.h +++ b/sql/log.h @@ -587,7 +587,14 @@ class MYSQL_BIN_LOG: public TC_LOG, private MYSQL_LOG long xid_count; /* For linking in requests to the binlog background thread. */ xid_count_per_binlog *next_in_queue;

- xid_count_per_binlog(); /* Give link error if constructor used. */

Maybe we should leave it as it seems to be for catching inadvertent object constructions. I'm cc-ing Kristian to clear out.

+ xid_count_per_binlog(char *log_file_name, uint log_file_name_len) + :binlog_name(log_file_name), binlog_name_len(log_file_name_len), + binlog_id(0), xid_count(0) + {} + ~xid_count_per_binlog() + { + my_free(binlog_name);

[*]

+ } }; I_List<xid_count_per_binlog> binlog_xid_count_list; mysql_mutex_t LOCK_binlog_background_thread; _______________________________________________ commits mailing list commits@mariadb.org https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits