From bddbef3573349b0565c43c27beba47c89358f39f Mon Sep 17 00:00:00 2001 From: Monty Date: Tue, 1 Oct 2024 17:07:48 +0300 Subject: [PATCH] MDEV-34533 asan error about stack overflow when writing record in Aria The problem was that when using clang + asan, we do not get a correct value for the thread stack as some local variables are not allocated at the normal stack. It looks like that for example clang 18.1.3, when compiling with -O2 -fsanitize=addressan it puts local variables and things allocated by alloca() in other areas than on the stack. The following code shows the issue Thread 6 "mariadbd" hit Breakpoint 3, do_handle_one_connection (connect=0x5080000027b8, put_in_cache=) at sql/sql_connect.cc:1399 THD *thd; 1399 thd->thread_stack= (char*) &thd; (gdb) p &thd (THD **) 0x7fffedee7060 (gdb) p $sp (void *) 0x7fffef4e7bc0 The address of thd is 24M away from the stack pointer (gdb) info reg ... rsp 0x7fffef4e7bc0 0x7fffef4e7bc0 ... r13 0x7fffedee7060 140737185214560 r13 is pointing to the address of the thd. Probably some kind of "local stack" used by the sanitizer I have verified this with gdb on a recursive call that calls alloca() in a loop. In this case all objects was stored in a local heap, not on the stack. To solve this issue in a portable way, I have added two functions: my_get_stack_pointer() returns the address of the current stack pointer. The code is using asm instructions for intel 32/64 bit, powerpc, arm 32/64 bit and sparc 32/64 bit. Supported compilers are gcc, clang and MSVC. For MSVC 64 bit we are using _AddressOfReturnAddress() As a fallback for other compilers/arch we use the address of a local variable. my_get_stack_bounds() that will return the address of the base stack and stack size using pthread_attr_getstack() or NtCurrentTed() with fallback to using the address of a local variable and user provided stack size. Server changes are: - Moving setting of thread_stack to THD::store_globals() using my_get_stack_bounds(). - Removing setting of thd->thread_stack, except in functions that allocates a lot on the stack before calling store_globals(). When using estimates for stack start, we reduce stack_size with MY_STACK_SAFE_MARGIN (8192) to take into account the stack used before calling store_globals(). I also added a unittest, stack_allocation-t, to verify the new code. Reviewed-by: Sergei Golubchik --- config.h.cmake | 1 + configure.cmake | 1 + include/my_stack_alloc.h | 84 +++++++++++++++++-- mysys/CMakeLists.txt | 2 +- mysys/my_stack.c | 95 ++++++++++++++++++++++ sql/event_scheduler.cc | 7 +- sql/events.cc | 6 -- sql/log.cc | 1 - sql/rpl_mi.cc | 1 - sql/rpl_parallel.cc | 1 - sql/semisync_master_ack_receiver.cc | 1 - sql/slave.cc | 11 ++- sql/sql_acl.cc | 2 - sql/sql_base.cc | 4 +- sql/sql_class.cc | 16 ++-- sql/sql_class.h | 6 +- sql/sql_connect.cc | 10 --- sql/sql_insert.cc | 1 - sql/sql_parse.cc | 5 +- sql/sql_plugin.cc | 2 +- sql/sql_prepare.cc | 3 +- sql/sql_reload.cc | 3 - sql/sql_servers.cc | 1 - sql/sql_table.cc | 1 - sql/sql_udf.cc | 2 +- sql/threadpool_common.cc | 1 - sql/tztime.cc | 2 +- sql/wsrep_mysqld.cc | 1 - sql/wsrep_schema.cc | 1 - sql/wsrep_server_service.cc | 2 +- sql/wsrep_sst.cc | 1 - sql/wsrep_thd.cc | 4 +- sql/wsrep_utils.cc | 1 - unittest/mysys/CMakeLists.txt | 2 +- unittest/mysys/stack_allocation-t.c | 120 ++++++++++++++++++++++++++++ 35 files changed, 328 insertions(+), 74 deletions(-) create mode 100644 mysys/my_stack.c create mode 100644 unittest/mysys/stack_allocation-t.c diff --git a/config.h.cmake b/config.h.cmake index 2c421798c33..2d26fc6b66a 100644 --- a/config.h.cmake +++ b/config.h.cmake @@ -195,6 +195,7 @@ #cmakedefine HAVE_PTHREAD_ATTR_GETSTACKSIZE 1 #cmakedefine HAVE_PTHREAD_ATTR_SETSCOPE 1 #cmakedefine HAVE_PTHREAD_ATTR_SETSTACKSIZE 1 +#cmakedefine HAVE_PTHREAD_GETATTR_NP 1 #cmakedefine HAVE_PTHREAD_CONDATTR_CREATE 1 #cmakedefine HAVE_PTHREAD_GETAFFINITY_NP 1 #cmakedefine HAVE_PTHREAD_KEY_DELETE 1 diff --git a/configure.cmake b/configure.cmake index 31e20aa664c..774e2554be8 100644 --- a/configure.cmake +++ b/configure.cmake @@ -379,6 +379,7 @@ CHECK_FUNCTION_EXISTS (pthread_attr_getguardsize HAVE_PTHREAD_ATTR_GETGUARDSIZE) CHECK_FUNCTION_EXISTS (pthread_attr_setstacksize HAVE_PTHREAD_ATTR_SETSTACKSIZE) CHECK_FUNCTION_EXISTS (pthread_condattr_create HAVE_PTHREAD_CONDATTR_CREATE) CHECK_FUNCTION_EXISTS (pthread_getaffinity_np HAVE_PTHREAD_GETAFFINITY_NP) +CHECK_FUNCTION_EXISTS (pthread_getattr_np HAVE_PTHREAD_GETATTR_NP) CHECK_FUNCTION_EXISTS (pthread_key_delete HAVE_PTHREAD_KEY_DELETE) CHECK_FUNCTION_EXISTS (pthread_rwlock_rdlock HAVE_PTHREAD_RWLOCK_RDLOCK) CHECK_FUNCTION_EXISTS (pthread_sigmask HAVE_PTHREAD_SIGMASK) diff --git a/include/my_stack_alloc.h b/include/my_stack_alloc.h index 95e746d6fe7..796bb69f877 100644 --- a/include/my_stack_alloc.h +++ b/include/my_stack_alloc.h @@ -17,6 +17,65 @@ #ifndef _my_stack_alloc_h #define _my_stack_alloc_h +#ifdef _MSC_VER +#include // For MSVC-specific intrinsics +#else +#include +#endif + + +/* + Get the address of the current stack. + This will fallback to using an estimate for the stack pointer + in the cases where either the compiler or the architecture is + not supported. +*/ + +static inline void *my_get_stack_pointer(void *default_stack) +{ + void *stack_ptr= NULL; + +#if defined(__GNUC__) || defined(__clang__) /* GCC and Clang compilers */ +#if defined(__i386__) /* Intel x86 (32-bit) */ + __asm__ volatile ("movl %%esp, %0" : "=r" (stack_ptr)); +#elif defined(__x86_64__) /* Intel x86-64 (64-bit) */ + __asm__ volatile ("movq %%rsp, %0" : "=r" (stack_ptr)); +#elif defined(__powerpc__) /* PowerPC (32-bit) */ + __asm__ volatile ("mr %0, 1" : "=r" (stack_ptr)); /* GPR1 is the stack pointer */ +#elif defined(__ppc64__) /* PowerPC (64-bit) */ + __asm__ volatile ("mr %0, 1" : "=r" (stack_ptr)); +#elif defined(__arm__) /* ARM 32-bit */ + __asm__ volatile ("mov %0, sp" : "=r" (stack_ptr)); +#elif defined(__aarch64__) /* ARM 64-bit */ + __asm__ volatile ("mov %0, sp" : "=r" (stack_ptr)); +#elif defined(__sparc__) /* SPARC 32-bit */ + __asm__ volatile ("mov %%sp, %0" : "=r" (stack_ptr)); +#elif defined(__sparc64__) /* SPARC 64-bit */ + __asm__ volatile ("mov %%sp, %0" : "=r" (stack_ptr)); +#elif defined(__s390x__) + stack_ptr= __builtin_frame_address(0); +#else + /* Generic fallback for unsupported architectures in GCC/Clang */ + stack_ptr= default_stack ? default_stack : (void*) &stack_ptr; +#endif +#elif defined(_MSC_VER) /* MSVC compiler (Intel only) */ +#if defined(_M_IX86) /* Intel x86 (32-bit) */ + __asm { mov stack_ptr, esp } +#elif defined(_M_X64) /* Intel x86-64 (64-bit) */ + /* rsp can’t be accessed directly in MSVC x64 */ + stack_ptr= _AddressOfReturnAddress(); +#else + /* Generic fallback for unsupported architectures in MSVC */ + stack_ptr= default_stack ? default_stack : (void*) &stack_ptr; +#endif +#else + /* Generic fallback for unsupported compilers */ + stack_ptr= default_stack ? default_stack : (void*) &stack_ptr; +#endif + return stack_ptr; +} + + /* Do allocation through alloca if there is enough stack available. If not, use my_malloc() instead. @@ -42,7 +101,7 @@ /* Allocate small blocks as long as there is this much left */ #define STACK_ALLOC_SMALL_BLOCK 1024*32 -/* Allocate small blocks as long as there is this much left */ +/* Allocate small blocks as long as the block size is not bigger than */ #define STACK_ALLOC_SMALL_BLOCK_SIZE 4096 /* @@ -56,11 +115,11 @@ do \ { \ size_t alloc_size= (size); \ - size_t stack_left= available_stack_size(&alloc_size, (stack_end)); \ - if (stack_left > alloc_size && \ - (STACK_ALLOC_BIG_BLOCK < stack_left - alloc_size || \ - ((STACK_ALLOC_SMALL_BLOCK < stack_left - alloc_size) && \ - (STACK_ALLOC_SMALL_BLOCK_SIZE <= alloc_size)))) \ + void *stack= my_get_stack_pointer(0); \ + size_t stack_left= available_stack_size(stack, (stack_end)); \ + if (stack_left > alloc_size + STACK_ALLOC_SMALL_BLOCK && \ + (stack_left > alloc_size + STACK_ALLOC_BIG_BLOCK || \ + (STACK_ALLOC_SMALL_BLOCK_SIZE >= alloc_size))) \ { \ (must_be_freed)= 0; \ (res)= alloca(size); \ @@ -90,3 +149,16 @@ static inline void stack_alloc_free(void *res, my_bool must_be_freed) my_free(res); } #endif /* _my_stack_alloc_h */ + + +/* Get start and end of stack */ + +/* + This is used in the case when we not know the exact stack start + and have to estimate stack start with get_stack_pointer() +*/ +#define MY_STACK_SAFE_MARGIN 8192 + +extern void my_get_stack_bounds(void **stack_start, void **stack_end, + void *fallback_stack_start, + size_t fallback_stack_size); diff --git a/mysys/CMakeLists.txt b/mysys/CMakeLists.txt index 39561c328f8..405c0deabcd 100644 --- a/mysys/CMakeLists.txt +++ b/mysys/CMakeLists.txt @@ -36,7 +36,7 @@ SET(MYSYS_SOURCES array.c charset-def.c charset.c my_default.c my_basename.c my_write.c ptr_cmp.c queues.c stacktrace.c string.c thr_alarm.c thr_lock.c thr_mutex.c - thr_rwlock.c thr_timer.c + thr_rwlock.c thr_timer.c my_stack.c tree.c typelib.c base64.c my_memmem.c my_getpagesize.c guess_malloc_library.c diff --git a/mysys/my_stack.c b/mysys/my_stack.c new file mode 100644 index 00000000000..3eec01092ac --- /dev/null +++ b/mysys/my_stack.c @@ -0,0 +1,95 @@ +/* Copyright 2024 MariaDB corporation AB + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA +*/ + + +/* + Get start and end of stack + + Note that the code depends on STACK_DIRECTION that is defined by cmake. + In general, STACK_DIRECTION should be 1 except in case of + an upward-growing stack that can happen on sparc or hpux platforms. +*/ + + +#include "my_global.h" +#include "my_sys.h" +#include "my_stack_alloc.h" + +#ifdef _WIN_ +#include +#include +#include +#endif + +/* Get start and end of stack */ + +extern void my_get_stack_bounds(void **stack_start, void **stack_end, + void *fallback_stack_start, + size_t fallback_stack_size) +{ +#if defined(__GNUC__) || defined(__clang__) /* GCC or Clang compilers */ + size_t stack_size; +#if defined(HAVE_PTHREAD_GETATTR_NP) + /* POSIX-compliant system (Linux, macOS, etc.) */ + pthread_attr_t attr; + pthread_t thread= pthread_self(); + void *stack_base; + + /* Get the thread attributes */ + if (pthread_getattr_np(thread, &attr) == 0) + { + /* Get stack base and size */ + pthread_attr_getstack(&attr, &stack_base, &stack_size); + /* + stack_base points to start of the stack region to which the + stack grows to + */ + *stack_start= stack_base - stack_size * STACK_DIRECTION; + pthread_attr_destroy(&attr); /* Clean up */ + } + else + { + /* + Fallback: + Use the current stack pointer as an approximation of the start + */ + *stack_start= my_get_stack_pointer(fallback_stack_start); + stack_size= (fallback_stack_size - + MY_MIN(fallback_stack_size, MY_STACK_SAFE_MARGIN)); + } +#else + /* Platform does not have pthread_getattr_np */ + *stack_start= my_get_stack_pointer(fallback_stack_start); + stack_size= (fallback_stack_size - + MY_MIN(fallback_stack_size, MY_STACK_SAFE_MARGIN)); +#endif /* defined(HAVE_PTHREAD_GETATTR_NP) */ + *stack_end= *stack_start + stack_size * STACK_DIRECTION; + +#elif defined(_MSC_VER) && defined(_WIN32) + /* Windows platform (MSVC) */ + NT_TIB* teb= (NT_TIB*)NtCurrentTeb(); + + *stack_start= teb->StackBase; /* Start of the stack */ + *stack_end= teb->StackLimit; /* End of the stack (stack limit) */ +#else + /* Unsupported platform / compiler */ + *stack_start= my_get_stack_pointer(fallback_stack_start); + *stack_end= (*stack_start + + (fallback_stack_size - + MY_MIN(fallback_stack_size, MY_STACK_SAFE_MARGIN)) * + STACK_DIRECTON); +#endif /* defined(__GNUC__) || defined(__clang__) */ +} diff --git a/sql/event_scheduler.cc b/sql/event_scheduler.cc index 9015c1b2655..a890c98c361 100644 --- a/sql/event_scheduler.cc +++ b/sql/event_scheduler.cc @@ -219,12 +219,10 @@ pre_init_event_thread(THD* thd) pthread_handler_t event_scheduler_thread(void *arg) { - /* needs to be first for thread_stack */ THD *thd= (THD *) ((struct scheduler_param *) arg)->thd; Event_scheduler *scheduler= ((struct scheduler_param *) arg)->scheduler; bool res; - - thd->thread_stack= (char *)&thd; // remember where our stack is + thd->reset_stack(); mysql_thread_set_psi_id(thd->thread_id); @@ -285,8 +283,6 @@ event_worker_thread(void *arg) void Event_worker_thread::run(THD *thd, Event_queue_element_for_exec *event) { - /* needs to be first for thread_stack */ - char my_stack; Event_job_data job_data; bool res; @@ -302,7 +298,6 @@ Event_worker_thread::run(THD *thd, Event_queue_element_for_exec *event) thd->charset(), NULL); #endif - thd->thread_stack= &my_stack; // remember where our stack is res= post_init_event_thread(thd); DBUG_ENTER("Event_worker_thread::run"); diff --git a/sql/events.cc b/sql/events.cc index 6fe0974430c..e7ea1cca5c6 100644 --- a/sql/events.cc +++ b/sql/events.cc @@ -901,12 +901,6 @@ Events::init(THD *thd, bool opt_noacl_or_bootstrap) res= TRUE; goto end; } - /* - The thread stack does not start from this function but we cannot - guess the real value. So better some value that doesn't assert than - no value. - */ - thd->thread_stack= (char*) &thd; thd->store_globals(); /* Set current time for the thread that handles events. diff --git a/sql/log.cc b/sql/log.cc index 4f74900e59a..a0f865dc7e0 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -10461,7 +10461,6 @@ binlog_background_thread(void *arg __attribute__((unused))) thd= new THD(next_thread_id()); thd->system_thread= SYSTEM_THREAD_BINLOG_BACKGROUND; - thd->thread_stack= (char*) &thd; /* Set approximate stack start */ thd->store_globals(); thd->security_ctx->skip_grants(); thd->set_command(COM_DAEMON); diff --git a/sql/rpl_mi.cc b/sql/rpl_mi.cc index 3c6db4f0aad..7a9dcd49493 100644 --- a/sql/rpl_mi.cc +++ b/sql/rpl_mi.cc @@ -1111,7 +1111,6 @@ bool Master_info_index::init_all_master_info() } thd= new THD(next_thread_id()); /* Needed by start_slave_threads */ - thd->thread_stack= (char*) &thd; thd->store_globals(); reinit_io_cache(&index_file, READ_CACHE, 0L,0,0); diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc index 9c4222d7817..ca469e3b6c7 100644 --- a/sql/rpl_parallel.cc +++ b/sql/rpl_parallel.cc @@ -1195,7 +1195,6 @@ handle_rpl_parallel_thread(void *arg) my_thread_init(); thd = new THD(next_thread_id()); - thd->thread_stack = (char*)&thd; server_threads.insert(thd); set_current_thd(thd); pthread_detach_this_thread(); diff --git a/sql/semisync_master_ack_receiver.cc b/sql/semisync_master_ack_receiver.cc index a76e3447ac8..10604926bca 100644 --- a/sql/semisync_master_ack_receiver.cc +++ b/sql/semisync_master_ack_receiver.cc @@ -201,7 +201,6 @@ void Ack_receiver::run() sql_print_information("Starting ack receiver thread"); thd->system_thread= SYSTEM_THREAD_SEMISYNC_MASTER_BACKGROUND; - thd->thread_stack= (char*) &thd; thd->store_globals(); thd->security_ctx->skip_grants(); thd->set_command(COM_DAEMON); diff --git a/sql/slave.cc b/sql/slave.cc index 1a6a5a6efdb..27dfcbf162d 100644 --- a/sql/slave.cc +++ b/sql/slave.cc @@ -367,7 +367,6 @@ end: static THD *new_bg_THD() { THD *thd= new THD(next_thread_id()); - thd->thread_stack= (char*) &thd; thd->store_globals(); thd->system_thread = SYSTEM_THREAD_SLAVE_BACKGROUND; thd->security_ctx->skip_grants(); @@ -628,7 +627,6 @@ int init_slave() { int error; THD *thd= new THD(next_thread_id()); - thd->thread_stack= (char*) &thd; thd->store_globals(); error= start_slave_threads(0, /* No active thd */ @@ -4724,7 +4722,12 @@ pthread_handler_t handle_slave_io(void *arg) thd->set_psi(PSI_CALL_get_thread()); pthread_detach_this_thread(); - thd->thread_stack= (char*) &thd; // remember where our stack is + /* + Remember where our stack is. This is needed for this function as + there are a lot of stack variables. It will be fixed in store_globals() + called by init_slave_thread(). + */ + thd->thread_stack= (void*) &thd; // remember where our stack is mi->clear_error(); if (init_slave_thread(thd, mi, SLAVE_THD_IO)) { @@ -5348,7 +5351,7 @@ pthread_handler_t handle_slave_sql(void *arg) serial_rgi= new rpl_group_info(rli); thd = new THD(next_thread_id()); // note that contructor of THD uses DBUG_ ! - thd->thread_stack = (char*)&thd; // remember where our stack is + thd->thread_stack= (void*) &thd; // Big stack, remember where our stack is thd->system_thread_info.rpl_sql_info= &sql_info; DBUG_ASSERT(rli->inited); diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index cd579be8010..28858277308 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -2509,7 +2509,6 @@ bool acl_init(bool dont_read_acl_tables) */ if (!(thd=new THD(0))) DBUG_RETURN(1); /* purecov: inspected */ - thd->thread_stack= (char*) &thd; thd->store_globals(); /* It is safe to call acl_reload() since acl_* arrays and hashes which @@ -7942,7 +7941,6 @@ bool grant_init() if (!(thd= new THD(0))) DBUG_RETURN(1); /* purecov: deadcode */ - thd->thread_stack= (char*) &thd; thd->store_globals(); return_val= grant_reload(thd); delete thd; diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 29daca40acb..0bfae16b23e 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8985,17 +8985,17 @@ fill_record_n_invoke_before_triggers(THD *thd, TABLE *table, Field **ptr, my_bool mysql_rm_tmp_tables(void) { + THD *thd; uint i, idx; char path[FN_REFLEN], *tmpdir, path_copy[FN_REFLEN]; MY_DIR *dirp; FILEINFO *file; TABLE_SHARE share; - THD *thd; DBUG_ENTER("mysql_rm_tmp_tables"); if (!(thd= new THD(0))) DBUG_RETURN(1); - thd->thread_stack= (char*) &thd; + thd->thread_stack= (void*) &thd; // Big stack thd->store_globals(); for (i=0; i<=mysql_tmpdir_list.max; i++) diff --git a/sql/sql_class.cc b/sql/sql_class.cc index fc04f5436f3..de688b4cb31 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -2202,12 +2202,6 @@ void THD::reset_killed() void THD::store_globals() { - /* - Assert that thread_stack is initialized: it's necessary to be able - to track stack overrun. - */ - DBUG_ASSERT(thread_stack); - set_current_thd(this); /* mysys_var is concurrently readable by a killer thread. @@ -2239,8 +2233,11 @@ void THD::store_globals() os_thread_id= 0; #endif real_id= pthread_self(); // For debugging - mysys_var->stack_ends_here= thread_stack + // for consistency, see libevent_thread_proc - STACK_DIRECTION * (long)my_thread_stack_size; + + /* Set stack start and stack end */ + my_get_stack_bounds(&thread_stack, &mysys_var->stack_ends_here, + thread_stack, my_thread_stack_size); + if (net.vio) { net.thd= this; @@ -2252,6 +2249,7 @@ void THD::store_globals() thr_lock_info_init(&lock_info, mysys_var); } + /** Untie THD from current thread @@ -5018,7 +5016,6 @@ TABLE *find_fk_open_table(THD *thd, const char *db, size_t db_len, MYSQL_THD create_thd() { THD *thd= new THD(next_thread_id()); - thd->thread_stack= (char*) &thd; thd->store_globals(); thd->set_command(COM_DAEMON); thd->system_thread= SYSTEM_THREAD_GENERIC; @@ -5095,7 +5092,6 @@ void *thd_attach_thd(MYSQL_THD thd) auto save_mysysvar= pthread_getspecific(THR_KEY_mysys); pthread_setspecific(THR_KEY_mysys, thd->mysys_var); - thd->thread_stack= (char *) &thd; thd->store_globals(); return save_mysysvar; } diff --git a/sql/sql_class.h b/sql/sql_class.h index 6baf796fb8b..e1c6b83c448 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2526,7 +2526,7 @@ public: A pointer to the stack frame of handle_one_connection(), which is called first in the thread for handling a client */ - char *thread_stack; + void *thread_stack; /** Currently selected catalog. @@ -3596,6 +3596,10 @@ public: void free_connection(); void reset_for_reuse(); void store_globals(); + void reset_stack() + { + thread_stack= 0; + } void reset_globals(); bool trace_started() { diff --git a/sql/sql_connect.cc b/sql/sql_connect.cc index 824f68890c0..825491cad44 100644 --- a/sql/sql_connect.cc +++ b/sql/sql_connect.cc @@ -1387,16 +1387,6 @@ void do_handle_one_connection(CONNECT *connect, bool put_in_cache) thd->thr_create_utime= thr_create_utime; /* We need to set this because of time_out_user_resource_limits */ thd->start_utime= thr_create_utime; - - /* - handle_one_connection() is normally the only way a thread would - start and would always be on the very high end of the stack , - therefore, the thread stack always starts at the address of the - first local variable of handle_one_connection, which is thd. We - need to know the start of the stack so that we could check for - stack overruns. - */ - thd->thread_stack= (char*) &thd; setup_connection_thread_globals(thd); for (;;) diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index d635ba7085b..2dccf7af587 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -3219,7 +3219,6 @@ pthread_handler_t handle_delayed_insert(void *arg) else { DBUG_ENTER("handle_delayed_insert"); - thd->thread_stack= (char*) &thd; if (init_thr_lock()) { thd->get_stmt_da()->set_error_status(ER_OUT_OF_RESOURCES); diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index e0c87ec2117..0ed938eb63e 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -996,7 +996,6 @@ int bootstrap(MYSQL_FILE *file) #endif /* The following must be called before DBUG_ENTER */ - thd->thread_stack= (char*) &thd; thd->store_globals(); thd->security_ctx->user= (char*) my_strdup(key_memory_MPVIO_EXT_auth_info, @@ -7630,7 +7629,9 @@ check_stack_overrun(THD *thd, long margin, uchar *buf __attribute__((unused))) #ifndef __SANITIZE_ADDRESS__ long stack_used; DBUG_ASSERT(thd == current_thd); - if ((stack_used= available_stack_size(thd->thread_stack, &stack_used)) >= + DBUG_ASSERT(thd->thread_stack); + if ((stack_used= available_stack_size(thd->thread_stack, + my_get_stack_pointer(&stack_used))) >= (long) (my_thread_stack_size - margin)) { thd->is_fatal_error= 1; diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc index 22767d3025e..5f58c6707b7 100644 --- a/sql/sql_plugin.cc +++ b/sql/sql_plugin.cc @@ -1887,7 +1887,7 @@ static void plugin_load(MEM_ROOT *tmp_root) if (global_system_variables.log_warnings >= 9) sql_print_information("Initializing installed plugins"); - new_thd->thread_stack= (char*) &tables; + new_thd->thread_stack= (void*) &tables; // Big stack new_thd->store_globals(); new_thd->db= MYSQL_SCHEMA_NAME; bzero((char*) &new_thd->net, sizeof(new_thd->net)); diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc index 405464c119b..8647db0e766 100644 --- a/sql/sql_prepare.cc +++ b/sql/sql_prepare.cc @@ -6133,7 +6133,7 @@ loc_advanced_command(MYSQL *mysql, enum enum_server_command command, { THD *thd_orig= current_thd; set_current_thd(p->thd); - p->thd->thread_stack= (char*) &result; + p->thd->thread_stack= (void*) &result; // Big stack p->thd->set_time(); result= execute_server_code(p->thd, (const char *)arg, arg_length); p->thd->cleanup_after_query(); @@ -6313,7 +6313,6 @@ extern "C" MYSQL *mysql_real_connect_local(MYSQL *mysql) new_thd= new THD(0); local_connection_thread_count++; - new_thd->thread_stack= (char*) &thd_orig; new_thd->store_globals(); new_thd->security_ctx->skip_grants(); new_thd->query_cache_is_applicable= 0; diff --git a/sql/sql_reload.cc b/sql/sql_reload.cc index 3a9b8e4641d..dd81907b150 100644 --- a/sql/sql_reload.cc +++ b/sql/sql_reload.cc @@ -88,10 +88,7 @@ bool reload_acl_and_cache(THD *thd, unsigned long long options, allocate temporary THD for execution of acl_reload()/grant_reload(). */ if (unlikely(!thd) && (thd= (tmp_thd= new THD(0)))) - { - thd->thread_stack= (char*) &tmp_thd; thd->store_globals(); - } if (likely(thd)) { diff --git a/sql/sql_servers.cc b/sql/sql_servers.cc index d63b0d66a5e..6e59feddc3b 100644 --- a/sql/sql_servers.cc +++ b/sql/sql_servers.cc @@ -251,7 +251,6 @@ bool servers_init(bool dont_read_servers_table) */ if (!(thd=new THD(0))) DBUG_RETURN(TRUE); - thd->thread_stack= (char*) &thd; thd->store_globals(); /* It is safe to call servers_reload() since servers_* arrays and hashes which diff --git a/sql/sql_table.cc b/sql/sql_table.cc index 4425157047c..c0f2502d61d 100644 --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -1686,7 +1686,6 @@ void execute_ddl_log_recovery() */ if (!(thd=new THD(0))) DBUG_VOID_RETURN; - thd->thread_stack= (char*) &thd; thd->store_globals(); thd->set_query(recover_query_string, strlen(recover_query_string)); diff --git a/sql/sql_udf.cc b/sql/sql_udf.cc index 02f068e9bbc..f5eab105335 100644 --- a/sql/sql_udf.cc +++ b/sql/sql_udf.cc @@ -179,7 +179,7 @@ void udf_init() DBUG_VOID_RETURN; } initialized = 1; - new_thd->thread_stack= (char*) &new_thd; + new_thd->thread_stack= (void*) &new_thd; // Big stack new_thd->store_globals(); new_thd->set_db(&MYSQL_SCHEMA_NAME); diff --git a/sql/threadpool_common.cc b/sql/threadpool_common.cc index 7c800696344..b5ae855deff 100644 --- a/sql/threadpool_common.cc +++ b/sql/threadpool_common.cc @@ -152,7 +152,6 @@ static void thread_attach(THD* thd) wsrep_wait_rollback_complete_and_acquire_ownership(thd); #endif /* WITH_WSREP */ set_mysys_var(thd->mysys_var); - thd->thread_stack=(char*)&thd; thd->store_globals(); PSI_CALL_set_thread(thd->get_psi()); mysql_socket_set_thread_owner(thd->net.vio->mysql_socket); diff --git a/sql/tztime.cc b/sql/tztime.cc index 9885fa88cf8..b8f3846a06c 100644 --- a/sql/tztime.cc +++ b/sql/tztime.cc @@ -1621,7 +1621,7 @@ my_tz_init(THD *org_thd, const char *default_tzname, my_bool bootstrap) */ if (!(thd= new THD(0))) DBUG_RETURN(1); - thd->thread_stack= (char*) &thd; + thd->thread_stack= (void*) &thd; // Big stack thd->store_globals(); /* Init all memory structures that require explicit destruction */ diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc index bc6c185c8d0..1be48c7c120 100644 --- a/sql/wsrep_mysqld.cc +++ b/sql/wsrep_mysqld.cc @@ -3301,7 +3301,6 @@ void* start_wsrep_THD(void *arg) (long long)thd->thread_id)); /* now that we've called my_thread_init(), it is safe to call DBUG_* */ - thd->thread_stack= (char*) &thd; wsrep_assign_from_threadvars(thd); wsrep_store_threadvars(thd); diff --git a/sql/wsrep_schema.cc b/sql/wsrep_schema.cc index 4cb839053c3..08e516b2a7a 100644 --- a/sql/wsrep_schema.cc +++ b/sql/wsrep_schema.cc @@ -692,7 +692,6 @@ int Wsrep_schema::init() WSREP_ERROR("Unable to get thd"); DBUG_RETURN(1); } - thd->thread_stack= (char*)&thd; wsrep_init_thd_for_schema(thd); if (Wsrep_schema_impl::execute_SQL(thd, create_cluster_table_str.c_str(), diff --git a/sql/wsrep_server_service.cc b/sql/wsrep_server_service.cc index 6184ba2df59..a2879b59789 100644 --- a/sql/wsrep_server_service.cc +++ b/sql/wsrep_server_service.cc @@ -34,7 +34,7 @@ #include "sql_base.h" /* close_thread_tables */ #include "debug_sync.h" -static void init_service_thd(THD* thd, char* thread_stack) +static void init_service_thd(THD* thd, void* thread_stack) { thd->thread_stack= thread_stack; thd->real_id= pthread_self(); diff --git a/sql/wsrep_sst.cc b/sql/wsrep_sst.cc index e9d2e158e39..cb2d61b7543 100644 --- a/sql/wsrep_sst.cc +++ b/sql/wsrep_sst.cc @@ -740,7 +740,6 @@ err: unireg_abort(1); } - thd->thread_stack= (char*) &thd; thd->security_ctx->skip_grants(); thd->system_thread= SYSTEM_THREAD_GENERIC; thd->real_id= pthread_self(); diff --git a/sql/wsrep_thd.cc b/sql/wsrep_thd.cc index f27b13b5874..1c51d05c56a 100644 --- a/sql/wsrep_thd.cc +++ b/sql/wsrep_thd.cc @@ -162,7 +162,7 @@ static void wsrep_rollback_high_priority(THD *thd, THD *rollbacker) { WSREP_DEBUG("Rollbacker aborting SR applier thd (%llu %lu)", thd->thread_id, thd->real_id); - char* orig_thread_stack= thd->thread_stack; + void* orig_thread_stack= thd->thread_stack; thd->thread_stack= rollbacker->thread_stack; DBUG_ASSERT(thd->wsrep_cs().mode() == Wsrep_client_state::m_high_priority); /* Must be streaming and must have been removed from the @@ -193,7 +193,7 @@ static void wsrep_rollback_local(THD *thd, THD *rollbacker) { WSREP_DEBUG("Rollbacker aborting local thd (%llu %lu)", thd->thread_id, thd->real_id); - char* orig_thread_stack= thd->thread_stack; + void* orig_thread_stack= thd->thread_stack; thd->thread_stack= rollbacker->thread_stack; if (thd->wsrep_trx().is_streaming()) { diff --git a/sql/wsrep_utils.cc b/sql/wsrep_utils.cc index a679304c40a..fb14f59bc86 100644 --- a/sql/wsrep_utils.cc +++ b/sql/wsrep_utils.cc @@ -421,7 +421,6 @@ thd::thd (my_bool won, bool system_thread) : init(), ptr(new THD(0)) { if (ptr) { - ptr->thread_stack= (char*) &ptr; wsrep_assign_from_threadvars(ptr); wsrep_store_threadvars(ptr); ptr->variables.option_bits&= ~OPTION_BIN_LOG; // disable binlog diff --git a/unittest/mysys/CMakeLists.txt b/unittest/mysys/CMakeLists.txt index 9dd5e5dbd9c..e35f9d72062 100644 --- a/unittest/mysys/CMakeLists.txt +++ b/unittest/mysys/CMakeLists.txt @@ -15,7 +15,7 @@ MY_ADD_TESTS(bitmap base64 my_atomic my_rdtsc lf my_malloc my_getopt dynstring byte_order - queues stacktrace crc32 LINK_LIBRARIES mysys) + queues stacktrace stack_allocation crc32 LINK_LIBRARIES mysys) MY_ADD_TESTS(my_vsnprintf LINK_LIBRARIES strings mysys) MY_ADD_TESTS(aes LINK_LIBRARIES mysys mysys_ssl) ADD_DEFINITIONS(${SSL_DEFINES}) diff --git a/unittest/mysys/stack_allocation-t.c b/unittest/mysys/stack_allocation-t.c new file mode 100644 index 00000000000..911bbd8de7a --- /dev/null +++ b/unittest/mysys/stack_allocation-t.c @@ -0,0 +1,120 @@ +/* Copyright (c) 2024, MariaDB Corporation. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; version 2 of the License. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, write to the Free Software + Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA */ + +#include +#include +#include +#include +#include +#include + +/* + Test of stack detection + The test is run with a stacks of STACK_ALLOC_SMALL_BLOCK_SIZE+1 and + STACK_ALLOC_SMALL_BLOCK_SIZE-1. + This is becasue of the function alloc_on_stack() has + different limits of how much it will allocate from the stack + based on the allocation size. +*/ + +/* + Common stack size in MariaDB. Cannot be bigger than system default + stack (common is 8M) +*/ +size_t my_stack_size= 299008; +size_t stack_allocation_total= 0; +extern long call_counter; +long call_counter; + +ATTRIBUTE_NOINLINE +int test_stack(void *stack_start, void *stack_end, int iteration, size_t stack_allocation) +{ + void *res, *stack; + my_bool must_be_freed; + + stack= my_get_stack_pointer(&must_be_freed); + if (stack_start < stack_end) + { + if (stack < stack_start || stack > stack_end) + return 1; + } + else + { + if (stack < stack_end || stack > stack_start) + return 1; + } + alloc_on_stack(stack_end, res, must_be_freed, stack_allocation); + bfill(res, stack_allocation, (char) iteration); + if (!must_be_freed) + { + stack_allocation_total+= stack_allocation; + test_stack(stack_start, stack_end, iteration+1, stack_allocation); + } + else + my_free(res); /* Was allocated with my_malloc */ + call_counter++; /* Avoid tail recursion optimization */ + return 0; +} + +void test_stack_detection(int stage, size_t stack_allocation) +{ + void *stack_start, *stack_end; + int res; + my_get_stack_bounds(&stack_start, &stack_end, + (void*) &stack_start, my_stack_size); + stack_allocation_total= 0; + res= test_stack(stack_start, stack_end, 1, stack_allocation); + if (!res) + ok(1, "%ld bytes allocated on stack of size %ld with %ld alloc size", + stack_allocation_total, + (long) available_stack_size(stack_start, stack_end), + (long) stack_allocation); + else + ok(0, "stack checking failed"); +} + +pthread_handler_t thread_stack_check(void *arg __attribute__((unused))) +{ + my_thread_init(); + test_stack_detection(1, STACK_ALLOC_SMALL_BLOCK_SIZE-1); + test_stack_detection(2, STACK_ALLOC_SMALL_BLOCK_SIZE+1); + my_thread_end(); + pthread_exit(0); + return 0; +} + +int main(int argc __attribute__((unused)), char **argv) +{ + pthread_attr_t thr_attr; + pthread_t check_thread; + void *value; + + MY_INIT(argv[0]); + + plan(4); + test_stack_detection(3, STACK_ALLOC_SMALL_BLOCK_SIZE-1); + test_stack_detection(4, STACK_ALLOC_SMALL_BLOCK_SIZE+1); + + /* Create a thread and run the same test */ + (void) pthread_attr_init(&thr_attr); + pthread_attr_setscope(&thr_attr,PTHREAD_SCOPE_SYSTEM); + (void) my_setstacksize(&thr_attr, my_stack_size); + pthread_create(&check_thread, &thr_attr, thread_stack_check, 0); + pthread_join(check_thread, &value); + (void) pthread_attr_destroy(&thr_attr); + + my_end(0); + return exit_status(); +}