Merge lp:~vlad-lesin/percona-server/5.5-bug-1351148 into lp:percona-server/5.5

[Vlad Lesin]

Bug #1351128 fix.

  The bug's summary:

  1) The value in pins->stack_ends_here is actually incorrect. This value is
  calculated when the thread is initialized in my_thread_init(). The problem is
  that the value doesn't take into account existing stack usage and just adds the
  thread stack size to calculate the beginning of the stack.

  2) _lf_pinbox_real_free() is using alloca() in a very unsafe way. alloca()
  should only be used by leaf functions, since you cannot predict the amount of
  stack space that non-leaf functions will use. As pins->stack_ends_here is
  calculated in wrong way and there is no window for the qsort() which is invoked
  from _lf_pinbox_real_free() after alloca(), stack overflow can take place.

  The solution:

  1) Take into accout the current stack offset when stack size is calculated
  in my_thread_init();

  2) Don't allocate the whole free stack space in _lf_pinbox_real_free(),
  take some window for qsort();

http://jenkins.percona.com/view/PS%205.5/job/percona-server-5.5-param/1049/

[Laurynas Biveinis (laurynas-biveinis)]

- How big is that "existing stack usage" in the thread by the time it's calculated?

-
[ 12%] Building C object mysys/CMakeFiles/mysys.dir/my_thr_init.c.o
/Users/laurynas/percona/lp-mysql-server/5.5-bug-1351148/mysys/my_thr_init.c:363:38: error: invalid operands to binary expression ('char *' and 'int')
  tmp->stack_ends_here= (char *)&tmp & (~(my_getpagesize() - 1)) +
                        ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~

review: Needs Fixing

[Vlad Lesin (vlad-lesin)]

> - How big is that "existing stack usage" in the thread by the time it's
> calculated?
About 4K.

(gdb) p (stack_addr + stack_size) - &error
$24 = 4825
(gdb) bt
#0 my_thread_init () at 5.6/mysys/my_thr_init.c:313
#1 0x0000000000aa8b6b in timer_notify_thread (arg=0x7fffffffd1c0)
    at 5.6/mysys/posix_timers.c:64
#2 0x00007ffff737d182 in start_thread (arg=0x7fffdd584700) at pthread_create.c:312
#3 0x00007ffff6889fbd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

> -
> [ 12%] Building C object mysys/CMakeFiles/mysys.dir/my_thr_init.c.o
> /Users/laurynas/percona/lp-mysql-
> server/5.5-bug-1351148/mysys/my_thr_init.c:363:38: error: invalid operands to
> binary expression ('char *' and 'int')
> tmp->stack_ends_here= (char *)&tmp & (~(my_getpagesize() - 1)) +
> ~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fixed.

[Laurynas Biveinis (laurynas-biveinis)]

    - I don't understand the stack_ends_here calculation for the
      #else case. In the first line we round down &tmp to the page
      size, and then we add either 0, either STACK_DIRECTION * stack
      size. Adding zero if STACK_DIRECTION == 1 does not look right?
      It seems the intention was to jump one page if the stack grows
      down, but for that the evaluation precedence is wrong, it needs
      extra parentheses around the ?: operator.

    - Related to the above, there is a warning regression with clang 6.0:

/Users/laurynas/percona/lp-mysql-server/5.5-bug-1351148/mysys/my_thr_init.c:371:58: warning: operator '?:' has lower precedence than '+'; '+' will be evaluated first [-Wparentheses]
                                  (STACK_DIRECTION == 1) ? 0 : my_getpagesize() +
                                  ~~~~~~~~~~~~~~~~~~~~~~ ^
/Users/laurynas/percona/lp-mysql-server/5.5-bug-1351148/mysys/my_thr_init.c:371:58: note: place parentheses around the '+' expression to silence this warning
                                  (STACK_DIRECTION == 1) ? 0 : my_getpagesize() +
                                                         ^
/Users/laurynas/percona/lp-mysql-server/5.5-bug-1351148/mysys/my_thr_init.c:371:58: note: place parentheses around the '?:' expression to evaluate it first
                                  (STACK_DIRECTION == 1) ? 0 : my_getpagesize() +

review: Needs Fixing

[Vlad Lesin (vlad-lesin)]

> - I don't understand the stack_ends_here calculation for the
> #else case. In the first line we round down &tmp to the page
> size, and then we add either 0, either STACK_DIRECTION * stack
> size. Adding zero if STACK_DIRECTION == 1 does not look right?
> It seems the intention was to jump one page if the stack grows
> down, but for that the evaluation precedence is wrong, it needs
> extra parentheses around the ?: operator.

Fixed.

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

[Laurynas Biveinis (laurynas-biveinis)]

The ?: vs + precedence issue is not fixed, only the warning is silenced, thus STACK_DIRECTION == 1 case still adds zero.

review: Needs Fixing

[Vlad Lesin (vlad-lesin)]

> The ?: vs + precedence issue is not fixed, only the warning is silenced, thus
> STACK_DIRECTION == 1 case still adds zero.
Yep. Fixed now.

[Laurynas Biveinis (laurynas-biveinis)]

review: Approve

=== modified file 'mysys/lf_alloc-pin.c'
--- mysys/lf_alloc-pin.c	2012-03-28 15:54:30 +0000
+++ mysys/lf_alloc-pin.c	2014-12-02 08:26:06 +0000
@@ -105,6 +105,13 @@
 
 #define LF_PINBOX_MAX_PINS 65536
 
+/*
+  When allocation on stack happens leave this
+  amount of stack memory for further functions
+  call.
+*/
+#define LF_PINBOX_EXTRA_STACK_SPACE  8192
+
 static void _lf_pinbox_real_free(LF_PINS *pins);
 
 /*
@@ -349,7 +356,9 @@
   {
     int alloca_size= sizeof(void *)*LF_PINBOX_PINS*npins;
     /* create a sorted list of pinned addresses, to speed up searches */
-    if (available_stack_size(&pinbox, *pins->stack_ends_here) > alloca_size)
+    if (available_stack_size(&pinbox, *pins->stack_ends_here) >
+        /* leave LF_PINBOX_EXTRA_STACK_SPACE bytes for qsort() call */
+        (alloca_size + LF_PINBOX_EXTRA_STACK_SPACE))
     {
       struct st_harvester hv;
       addr= (void **) alloca(alloca_size);
 
=== modified file 'mysys/my_thr_init.c'
--- mysys/my_thr_init.c	2014-04-23 09:46:00 +0000
+++ mysys/my_thr_init.c	2014-12-02 08:26:06 +0000
@@ -344,8 +344,34 @@
   mysql_mutex_init(key_my_thread_var_mutex, &tmp->mutex, MY_MUTEX_INIT_FAST);
   mysql_cond_init(key_my_thread_var_suspend, &tmp->suspend, NULL);
 
-  tmp->stack_ends_here= (char*)&tmp +
-                         STACK_DIRECTION * (long)my_thread_stack_size;
+#if defined(_GNU_SOURCE) && (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600)
+  {
+    pthread_attr_t attr;
+    char*          stack_addr;
+    size_t         stack_size;
+
+    if (pthread_attr_init(&attr) ||
+        pthread_getattr_np(tmp->pthread_self, &attr) ||
+        pthread_attr_getstack(&attr, (void**)&stack_addr, &stack_size))
+    {
+      error= 1;
+      goto end;
+    }
+    tmp->stack_ends_here= stack_addr;
+
+    if (pthread_attr_destroy(&attr))
+    {
+      error= 1;
+      goto end;
+    }
+  }
+#else  //defined(_GNU_SOURCE) && (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600)
+  /* assuming tmp is located within the first page of the stack */
+  tmp->stack_ends_here= (void **)(((long long)&tmp & (~((long long)my_getpagesize() - 1))) +
+                                  ((STACK_DIRECTION == 1) ?
+                                  my_thread_stack_size :
+                                  my_getpagesize() - my_thread_stack_size));
+#endif //defined(_GNU_SOURCE) && (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600)
 
   mysql_mutex_lock(&THR_LOCK_threads);
   tmp->id= ++thread_id;