MDEV-33161 Function pointer signature mismatch in LF_HASH

In cmake -DWITH_UBSAN=ON builds with clang but not with GCC,
-fsanitize=undefined will flag several runtime errors on
function pointer mismatch related to the lock-free hash table LF_HASH.

Let us use matching function signatures and remove function pointer
casts in order to avoid potential bugs due to undefined behaviour.

These errors could be caught at compilation time by
-Wcast-function-type-strict, which is available starting with clang-16,
but not available in any version of GCC as of now. The old GCC flag
-Wcast-function-type is enabled as part of -Wextra, but it specifically
does not catch these errors.

Reviewed by: Vladislav Vaintroub

Author: dr-m

mysys/lf_alloc-pin.c

@@ -242,8 +242,10 @@ void lf_pinbox_put_pins(LF_PINS *pins)
   return;
 }
 
-static int ptr_cmp(void **a, void **b)
+static int ptr_cmp(const void *pa, const void *pb)
 {
+  const void *const*a= pa;
+  const void *const*b= pb;
   return *a < *b ? -1 : *a == *b ? 0 : 1;
 }
 
@@ -283,8 +285,10 @@ struct st_harvester {
   callback forlf_dynarray_iterate:
   scan all pins of all threads and accumulate all pins
 */
-static int harvest_pins(LF_PINS *el, struct st_harvester *hv)
+static int harvest_pins(void *e, void *h)
 {
+  LF_PINS *el= e;
+  struct st_harvester *hv= h;
   int i;
   LF_PINS *el_end= el+MY_MIN(hv->npins, LF_DYNARRAY_LEVEL_LENGTH);
   for (; el < el_end; el++)
@@ -310,8 +314,9 @@ static int harvest_pins(LF_PINS *el, struct st_harvester *hv)
   callback forlf_dynarray_iterate:
   scan all pins of all threads and see if addr is present there
 */
-static int match_pins(LF_PINS *el, void *addr)
+static int match_pins(void *e, void *addr)
 {
+  LF_PINS *el= e;
   int i;
   LF_PINS *el_end= el+LF_DYNARRAY_LEVEL_LENGTH;
   for (; el < el_end; el++)
@@ -352,13 +357,12 @@ static void lf_pinbox_real_free(LF_PINS *pins)
       hv.granary= addr;
       hv.npins= npins;
       /* scan the dynarray and accumulate all pinned addresses */
-     lf_dynarray_iterate(&pinbox->pinarray,
-                           (lf_dynarray_func)harvest_pins, &hv);
+     lf_dynarray_iterate(&pinbox->pinarray, harvest_pins, &hv);
 
       npins= (int)(hv.granary-addr);
       /* and sort them */
       if (npins)
-        qsort(addr, npins, sizeof(void *), (qsort_cmp)ptr_cmp);
+        qsort(addr, npins, sizeof(void *), ptr_cmp);
     }
   }
 #endif
@@ -387,8 +391,7 @@ static void lf_pinbox_real_free(LF_PINS *pins)
       }
       else /* no alloca - no cookie. linear search here */
       {
-        if (lf_dynarray_iterate(&pinbox->pinarray,
-                                 (lf_dynarray_func)match_pins, cur))
+        if (lf_dynarray_iterate(&pinbox->pinarray, match_pins, cur))
           goto found;
       }
     }
@@ -416,10 +419,11 @@ static void lf_pinbox_real_free(LF_PINS *pins)
     'first' and 'last' are the ends of the linked list of nodes:
     first->el->el->....->el->last. Use first==last to free only one element.
 */
-static void alloc_free(uchar *first,
-                       uchar volatile *last,
-                       LF_ALLOCATOR *allocator)
+static void alloc_free(void *f, void *l, void *alloc)
 {
+  uchar *first= f;
+  uchar volatile *last= l;
+  LF_ALLOCATOR *allocator= alloc;
   /*
     we need a union here to access type-punned pointer reliably.
     otherwise gcc -fstrict-aliasing will not see 'tmp' changed in the loop
@@ -448,8 +452,7 @@ static void alloc_free(uchar *first,
 */
 void lf_alloc_init(LF_ALLOCATOR *allocator, uint size, uint free_ptr_offset)
 {
-  lf_pinbox_init(&allocator->pinbox, free_ptr_offset,
-                 (lf_pinbox_free_func *)alloc_free, allocator);
+  lf_pinbox_init(&allocator->pinbox, free_ptr_offset, alloc_free, allocator);
   allocator->top= 0;
   allocator->mallocs= 0;
   allocator->element_size= size;

mysys/thr_mutex.c

@@ -62,14 +62,10 @@ my_bool safe_mutex_deadlock_detector= 1;        /* On by default */
 static struct st_safe_mutex_create_info_t *safe_mutex_create_root= NULL;
 #endif
 
-static my_bool add_used_to_locked_mutex(safe_mutex_t *used_mutex,
-                                        safe_mutex_deadlock_t *locked_mutex);
-static my_bool add_to_locked_mutex(safe_mutex_deadlock_t *locked_mutex,
-                                   safe_mutex_t *current_mutex);
-static my_bool remove_from_locked_mutex(safe_mutex_t *mp,
-                                        safe_mutex_t *delete_mutex);
-static my_bool remove_from_used_mutex(safe_mutex_deadlock_t *locked_mutex,
-                                      safe_mutex_t *mutex);
+static my_bool add_used_to_locked_mutex(void *used, void *locked);
+static my_bool add_to_locked_mutex(void *locked, void *current);
+static my_bool remove_from_locked_mutex(void *m, void* remove);
+static my_bool remove_from_used_mutex(void *locked, void *m);
 static void print_deadlock_warning(safe_mutex_t *new_mutex,
                                    safe_mutex_t *conflicting_mutex);
 #endif
@@ -373,8 +369,7 @@ int safe_mutex_lock(safe_mutex_t *mp, myf my_flags, const char *file,
             are now locking (C) in B->C, then we would add C into
             B->locked_mutex and A->locked_mutex
           */
-          my_hash_iterate(mutex_root->used_mutex,
-                          (my_hash_walk_action) add_used_to_locked_mutex,
+          my_hash_iterate(mutex_root->used_mutex, add_used_to_locked_mutex,
                           deadlock);
 
           /*
@@ -654,12 +649,8 @@ void safe_mutex_free_deadlock_data(safe_mutex_t *mp)
   if (!(mp->create_flags & MYF_NO_DEADLOCK_DETECTION) && mp->used_mutex != NULL)
   {
     pthread_mutex_lock(&THR_LOCK_mutex);
-    my_hash_iterate(mp->used_mutex,
-                    (my_hash_walk_action) remove_from_locked_mutex,
-                    mp);
-    my_hash_iterate(mp->locked_mutex,
-                    (my_hash_walk_action) remove_from_used_mutex,
-                    mp);
+    my_hash_iterate(mp->used_mutex, remove_from_locked_mutex, mp);
+    my_hash_iterate(mp->locked_mutex, remove_from_used_mutex, mp);
     pthread_mutex_unlock(&THR_LOCK_mutex);
 
     my_hash_free(mp->used_mutex);
@@ -709,15 +700,15 @@ void safe_mutex_end(FILE *file __attribute__((unused)))
 #endif /* SAFE_MUTEX_DETECT_DESTROY */
 }
 
-static my_bool add_used_to_locked_mutex(safe_mutex_t *used_mutex,
-                                        safe_mutex_deadlock_t *locked_mutex)
+static my_bool add_used_to_locked_mutex(void *used, void *locked)
 {
+  safe_mutex_t *used_mutex= used;
+  safe_mutex_deadlock_t *locked_mutex= locked;
   /* Add mutex to all parent of the current mutex */
   if (!locked_mutex->warning_only)
   {
     (void) my_hash_iterate(locked_mutex->mutex->locked_mutex,
-                           (my_hash_walk_action) add_to_locked_mutex,
-                           used_mutex);
+                           add_to_locked_mutex, used_mutex);
     /* mark that locked_mutex is locked after used_mutex */
     (void) add_to_locked_mutex(locked_mutex, used_mutex);
   }
@@ -729,12 +720,13 @@ static my_bool add_used_to_locked_mutex(safe_mutex_t *used_mutex,
    register that locked_mutex was locked after current_mutex
 */
 
-static my_bool add_to_locked_mutex(safe_mutex_deadlock_t *locked_mutex,
-                                   safe_mutex_t *current_mutex)
+static my_bool add_to_locked_mutex(void *locked, void *current)
 {
+  safe_mutex_deadlock_t *locked_mutex= locked;
+  safe_mutex_t *current_mutex= current;
   DBUG_ENTER("add_to_locked_mutex");
-  DBUG_PRINT("info", ("inserting 0x%lx  into  0x%lx  (id: %lu -> %lu)",
-                      (ulong) locked_mutex, (long) current_mutex,
+  DBUG_PRINT("info", ("inserting %p  into  %p  (id: %lu -> %lu)",
+                      locked_mutex, current_mutex,
                       locked_mutex->id, current_mutex->id));
   if (my_hash_insert(current_mutex->locked_mutex, (uchar*) locked_mutex))
   {
@@ -762,13 +754,14 @@ static my_bool add_to_locked_mutex(safe_mutex_deadlock_t *locked_mutex,
     When counter goes to 0, we delete the safe_mutex_deadlock_t entry.
 */
 
-static my_bool remove_from_locked_mutex(safe_mutex_t *mp,
-                                        safe_mutex_t *delete_mutex)
+static my_bool remove_from_locked_mutex(void *m, void *remove)
 {
+  safe_mutex_t *mp= m;
+  safe_mutex_t *delete_mutex= remove;
   safe_mutex_deadlock_t *found;
   DBUG_ENTER("remove_from_locked_mutex");
-  DBUG_PRINT("enter", ("delete_mutex: 0x%lx  mutex: 0x%lx  (id: %lu <- %lu)",
-                       (ulong) delete_mutex, (ulong) mp, 
+  DBUG_PRINT("enter", ("delete_mutex: %p  mutex: %p  (id: %lu <- %lu)",
+                       delete_mutex, mp,
                        delete_mutex->id, mp->id));
 
   found= (safe_mutex_deadlock_t *) my_hash_search(mp->locked_mutex,
@@ -786,12 +779,13 @@ static my_bool remove_from_locked_mutex(safe_mutex_t *mp,
   DBUG_RETURN(0);
 }
 
-static my_bool remove_from_used_mutex(safe_mutex_deadlock_t *locked_mutex,
-                                      safe_mutex_t *mutex)
+static my_bool remove_from_used_mutex(void *locked, void *m)
 {
+  safe_mutex_deadlock_t *locked_mutex= locked;
+  safe_mutex_t *mutex= m;
   DBUG_ENTER("remove_from_used_mutex");
-  DBUG_PRINT("enter", ("delete_mutex: 0x%lx  mutex: 0x%lx  (id: %lu <- %lu)",
-                       (ulong) mutex, (ulong) locked_mutex, 
+  DBUG_PRINT("enter", ("delete_mutex: %p  mutex: %p  (id: %lu <- %lu)",
+                       mutex, locked_mutex,
                        mutex->id, locked_mutex->id));
   if (my_hash_delete(locked_mutex->mutex->used_mutex, (uchar*) mutex))
   {

mysys/waiting_threads.c

@@ -424,8 +424,10 @@ static void wt_resource_destroy(uchar *arg)
   It's called from lf_hash when an element is inserted.
 */
 static void wt_resource_init(LF_HASH *hash __attribute__((unused)),
-                             WT_RESOURCE *rc, WT_RESOURCE_ID *id)
+                             void *resource, const void *ident)
 {
+  WT_RESOURCE *rc= resource;
+  const WT_RESOURCE_ID *id= ident;
   DBUG_ENTER("wt_resource_init");
   rc->id= *id;
   rc->waiter_count= 0;

sql/mdl.cc

@@ -762,8 +762,10 @@ struct mdl_iterate_arg
 };
 
 
-static my_bool mdl_iterate_lock(MDL_lock *lock, mdl_iterate_arg *arg)
+static my_bool mdl_iterate_lock(void *lk, void *a)
 {
+  MDL_lock *lock= static_cast<MDL_lock*>(lk);
+  mdl_iterate_arg *arg= static_cast<mdl_iterate_arg*>(a);
   /*
     We can skip check for m_strategy here, becase m_granted
     must be empty for such locks anyway.
@@ -786,14 +788,13 @@ int mdl_iterate(mdl_iterator_callback callback, void *arg)
 {
   DBUG_ENTER("mdl_iterate");
   mdl_iterate_arg argument= { callback, arg };
-  LF_PINS *pins= mdl_locks.get_pins();
   int res= 1;
 
-  if (pins)
+  if (LF_PINS *pins= mdl_locks.get_pins())
   {
     res= mdl_iterate_lock(mdl_locks.m_backup_lock, &argument) ||
-         lf_hash_iterate(&mdl_locks.m_locks, pins,
-                         (my_hash_walk_action) mdl_iterate_lock, &argument);
+         lf_hash_iterate(&mdl_locks.m_locks, pins, mdl_iterate_lock,
+                         &argument);
     lf_hash_put_pins(pins);
   }
   DBUG_RETURN(res);

sql/sql_base.cc

@@ -252,9 +252,10 @@ class list_open_tables_arg
 };
 
 
-static my_bool list_open_tables_callback(TDC_element *element,
-                                         list_open_tables_arg *arg)
+static my_bool list_open_tables_callback(void *el, void *a)
 {
+  TDC_element *element= static_cast<TDC_element*>(el);
+  list_open_tables_arg *arg= static_cast<list_open_tables_arg*>(a);
   const Lex_ident_db
     db= Lex_ident_db(Lex_cstring_strlen((const char*) element->m_key));
   const char *table_name= db.str + db.length + 1;
@@ -302,8 +303,7 @@ OPEN_TABLE_LIST *list_open_tables(THD *thd,
   DBUG_ENTER("list_open_tables");
   list_open_tables_arg argument(thd, db, wild);
 
-  if (tdc_iterate(thd, (my_hash_walk_action) list_open_tables_callback,
-                  &argument, true))
+  if (tdc_iterate(thd, list_open_tables_callback, &argument, true))
     DBUG_RETURN(0);
 
   DBUG_RETURN(argument.open_list);
@@ -462,9 +462,10 @@ struct tc_collect_arg
   flush_tables_type flush_type;
 };
 
-static my_bool tc_collect_used_shares(TDC_element *element,
-                                      tc_collect_arg *arg)
+static my_bool tc_collect_used_shares(void *el, void *a)
 {
+  TDC_element *element= static_cast<TDC_element*>(el);
+  tc_collect_arg *arg= static_cast<tc_collect_arg*>(a);
   my_bool result= FALSE;
 
   DYNAMIC_ARRAY *shares= &arg->shares;
@@ -573,8 +574,7 @@ bool flush_tables(THD *thd, flush_tables_type flag)
   my_init_dynamic_array(PSI_INSTRUMENT_ME, &collect_arg.shares,
                         sizeof(TABLE_SHARE*), 100, 100, MYF(0));
   collect_arg.flush_type= flag;
-  if (tdc_iterate(thd, (my_hash_walk_action) tc_collect_used_shares,
-                  &collect_arg, true))
+  if (tdc_iterate(thd, tc_collect_used_shares, &collect_arg, true))
   {
     /* Release already collected shares */
     for (uint i= 0 ; i < collect_arg.shares.elements ; i++)

sql/sql_servers.cc

@@ -133,9 +133,10 @@ struct close_cached_connection_tables_arg
 };
 
 
-static my_bool close_cached_connection_tables_callback(
-  TDC_element *element, close_cached_connection_tables_arg *arg)
+static my_bool close_cached_connection_tables_callback(void *el, void *a)
 {
+  TDC_element *element= static_cast<TDC_element*>(el);
+  auto arg= static_cast<close_cached_connection_tables_arg*>(a);
   TABLE_LIST *tmp;
 
   mysql_mutex_lock(&element->LOCK_table_share);
@@ -188,9 +189,7 @@ static bool close_cached_connection_tables(THD *thd, LEX_CSTRING *connection)
   close_cached_connection_tables_arg argument= { thd, connection, 0 };
   DBUG_ENTER("close_cached_connections");
 
-  if (tdc_iterate(thd,
-                  (my_hash_walk_action) close_cached_connection_tables_callback,
-                  &argument))
+  if (tdc_iterate(thd, close_cached_connection_tables_callback, &argument))
     DBUG_RETURN(true);
 
   DBUG_RETURN(argument.tables ?

sql/sql_test.cc

@@ -116,7 +116,7 @@ static void print_cached_tables(void)
   /* purecov: begin tested */
   puts("DB             Table                            Version  Thread  Open  Lock");
 
-  tdc_iterate(0, (my_hash_walk_action) print_cached_tables_callback, NULL, true);
+  tdc_iterate(0, print_cached_tables_callback, NULL, true);
 
   fflush(stdout);
   /* purecov: end */

sql/table_cache.cc

@@ -309,7 +309,7 @@ void tc_purge()
 {
   Share_free_tables::List purge_tables;
 
-  tdc_iterate(0, (my_hash_walk_action) tc_purge_callback, &purge_tables);
+  tdc_iterate(0, tc_purge_callback, &purge_tables);
   while (auto table= purge_tables.pop_front())
     intern_close_table(table);
 }
@@ -1126,17 +1126,18 @@ struct eliminate_duplicates_arg
 
 
 static uchar *eliminate_duplicates_get_key(const uchar *element, size_t *length,
-                                       my_bool not_used __attribute__((unused)))
+                                           my_bool)
 {
   LEX_STRING *key= (LEX_STRING *) element;
   *length= key->length;
   return (uchar *) key->str;
 }
 
 
-static my_bool eliminate_duplicates(TDC_element *element,
-                                    eliminate_duplicates_arg *arg)
+static my_bool eliminate_duplicates(void *el, void *a)
 {
+  TDC_element *element= static_cast<TDC_element*>(el);
+  eliminate_duplicates_arg *arg= static_cast<eliminate_duplicates_arg*>(a);
   LEX_STRING *key= (LEX_STRING *) alloc_root(&arg->root, sizeof(LEX_STRING));
 
   if (!key || !(key->str= (char*) memdup_root(&arg->root, element->m_key,
@@ -1182,7 +1183,7 @@ int tdc_iterate(THD *thd, my_hash_walk_action action, void *argument,
                  hash_flags);
     no_dups_argument.action= action;
     no_dups_argument.argument= argument;
-    action= (my_hash_walk_action) eliminate_duplicates;
+    action= eliminate_duplicates;
     argument= &no_dups_argument;
   }