maria-developers team mailing list archive

[andrei.elkin@xxxxxxxxxx]

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@xxxxxxxxxxx>
     To: sujatha <sujatha.sivakumar@xxxxxxxxxxx>
     Cc: <commits@xxxxxxxxxxx>,  <maria-developers@xxxxxxxxxxxxxxxxxxx>
     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@xxxxxxxxxxx> 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@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits