[MDEV-15795] Stack exceeded if pthread_attr_setstacksize(&thr_attr,8196) succeeds Created: 2018-04-06  Updated: 2022-10-22  Resolved: 2022-10-22

Status: Closed
Project: MariaDB Server
Component/s: Server
Affects Version/s: 10.2
Fix Version/s: 10.3.37, 10.4.27, 10.5.18, 10.6.11, 10.7.7, 10.8.6, 10.9.4

Type: Bug Priority: Critical
Reporter: Vasil (Inactive) Assignee: Sergei Golubchik
Resolution: Fixed Votes: 0
Labels: None


 Description   

mysys/thr_timer.c contains this code:

 67 my_bool init_thr_timer(uint alloc_timers)
 68 {
 ...
 88   pthread_attr_setstacksize(&thr_attr,8196);
 ...
 90   if (mysql_thread_create(key_thread_timer, &timer_thread, &thr_attr,
 91                           timer_handler, NULL))

On Linux the pthread_attr_setstacksize() call on line 88 fails due to `EINVAL The stack size is less than PTHREAD_STACK_MIN (16384) bytes.`, the error is being ignored by the code and so the fact that the thread actually uses more than 8196 bytes of stack remains unnoticed.

On FreeBSD the call is accepted and the thread's stack size is indeed capped to 8196 bytes (btw, 2^13 is 8192, why 8196 is used!?). Then the stack is exceeded and a crash occurs during e.g. `mysqld --help --verbose`.

Apply this patch to demonstrate the problem, either by printing the bytes that were overwritten beyond the 8196-th byte or by causing a segfault the moment the overflow occurs:

diff --git i/mysys/thr_timer.c w/mysys/thr_timer.c
index b8726617f44..551b0a7b381 100644
--- i/mysys/thr_timer.c
+++ w/mysys/thr_timer.c
@@ -20,12 +20,13 @@
 */
 
 #include "mysys_priv.h"
 #include "thr_timer.h"
 #include <m_string.h>
 #include <queues.h>
+#include <stdint.h>
 #ifdef HAVE_TIMER_CREATE
 #include <sys/syscall.h>
 #endif
 
 struct timespec next_timer_expire_time;
 
@@ -61,12 +62,18 @@ static int compare_timespec(void *not_used __attribute__((unused)),
   @return 0 ok
   @return 1 error; Can't create thread
 */
 
 static thr_timer_t max_timer_data;
 
+static void* buf;
+const size_t malloc_size = 128*1024;
+const size_t pre_size = 16*1024;
+const size_t stack_size = 8196;
+const int magic_byte = 0xf7;
+
 my_bool init_thr_timer(uint alloc_timers)
 {
   pthread_attr_t thr_attr;
   my_bool res= 0;
   DBUG_ENTER("init_thr_timer");
 
@@ -82,13 +89,22 @@ my_bool init_thr_timer(uint alloc_timers)
   queue_insert(&timer_queue, (uchar*) &max_timer_data);
   next_timer_expire_time= max_timer_data.expire_time;
 
   /* Create a thread to handle timers */
   pthread_attr_init(&thr_attr);
   pthread_attr_setscope(&thr_attr,PTHREAD_SCOPE_PROCESS);
-  pthread_attr_setstacksize(&thr_attr,8196);
+
+  buf = malloc(malloc_size);
+  DBUG_ASSERT(buf != NULL);
+  memset(buf, magic_byte, malloc_size);
+  int ret;
+  //ret = mprotect(buf, pre_size, PROT_NONE);
+  //DBUG_ASSERT(ret == 0);
+  ret = pthread_attr_setstack(&thr_attr, (char*)buf + pre_size, stack_size);
+  DBUG_ASSERT(ret == 0);
+
   thr_timer_inited= 1;
   if (mysql_thread_create(key_thread_timer, &timer_thread, &thr_attr,
                           timer_handler, NULL))
   {
     thr_timer_inited= 0;
     res= 1;
@@ -112,12 +128,25 @@ void end_thr_timer(void)
   mysql_mutex_lock(&LOCK_timer);
   thr_timer_inited= 0;                          /* Signal abort */
   mysql_cond_signal(&COND_timer);
   mysql_mutex_unlock(&LOCK_timer);
   pthread_join(timer_thread, NULL);
 
+  for (size_t i = 0; i < pre_size; i++) {
+    const unsigned char c = *((unsigned char*)buf + i);
+    if (i > 0 && i % 16 == 0) {
+      fprintf(stderr, "\n");
+    }
+    if (c == magic_byte) {
+      fprintf(stderr, "__ ");
+    } else {
+      fprintf(stderr, "%02hhx ", c);
+    }
+  }
+  fprintf(stderr, "\n");
+
   mysql_mutex_destroy(&LOCK_timer);
   mysql_cond_destroy(&COND_timer);
   delete_queue(&timer_queue);
   DBUG_VOID_RETURN;
 }
 



 Comments   
Comment by Vasil (Inactive) [ 2018-04-06 ]

Notice: on amd64 architectures the stack grows from big addresses to small addresses, so the overwritten bytes are before the supplied buffer.

Why is `pthread_attr_setstacksize(&thr_attr,8196);` needed in the first place? I guess the fix of this bug is to just remove it. Capping the stack without a mechanism to catch overflow in a useful way leads to hard to diagnose crashes.

Comment by Daniel Black [ 2018-04-06 ]

Given its 18+ year old introduction (which I think is where init_thr_timer was copied from), I suspect the original reason isn't valid any more.
https://github.com/MariaDB/server/blame/e2ac8d3ff1fb941090fe63d3dfc55562f34326f4/mysys/my_pthread.c#L266

Comment by Vasil (Inactive) [ 2018-05-03 ]

Hi,

I think the fix for this is to just remove the pthread_attr_setstacksize() call.

Comment by Vladislav Vaintroub [ 2022-10-21 ]

Hi Serg, the patch looks good to me, ok to push.

Generated at Thu Feb 08 08:24:04 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.