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

Proposed by Vlad Lesin on 2014-09-23
Status: Merged
Approved by: Laurynas Biveinis on 2014-12-02
Approved revision: 692
Merged at revision: 719
Proposed branch: lp:~vlad-lesin/percona-server/5.5-bug-1351148
Merge into: lp:percona-server/5.5
Diff against target: 69 lines (+38/-3)
2 files modified
mysys/lf_alloc-pin.c (+10/-1)
mysys/my_thr_init.c (+28/-2)
To merge this branch: bzr merge lp:~vlad-lesin/percona-server/5.5-bug-1351148
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) Approve on 2014-12-02
Registry Administrators 2014-09-23 Pending
Review via email: mp+235723@code.launchpad.net

Description of the change

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/

To post a comment you must log in.

- 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) wrote :

> - 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.

    - 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) wrote :

> - 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.

review: Approve

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) wrote :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'mysys/lf_alloc-pin.c'
2--- mysys/lf_alloc-pin.c 2012-03-28 15:54:30 +0000
3+++ mysys/lf_alloc-pin.c 2014-12-02 08:26:06 +0000
4@@ -105,6 +105,13 @@
5
6 #define LF_PINBOX_MAX_PINS 65536
7
8+/*
9+ When allocation on stack happens leave this
10+ amount of stack memory for further functions
11+ call.
12+*/
13+#define LF_PINBOX_EXTRA_STACK_SPACE 8192
14+
15 static void _lf_pinbox_real_free(LF_PINS *pins);
16
17 /*
18@@ -349,7 +356,9 @@
19 {
20 int alloca_size= sizeof(void *)*LF_PINBOX_PINS*npins;
21 /* create a sorted list of pinned addresses, to speed up searches */
22- if (available_stack_size(&pinbox, *pins->stack_ends_here) > alloca_size)
23+ if (available_stack_size(&pinbox, *pins->stack_ends_here) >
24+ /* leave LF_PINBOX_EXTRA_STACK_SPACE bytes for qsort() call */
25+ (alloca_size + LF_PINBOX_EXTRA_STACK_SPACE))
26 {
27 struct st_harvester hv;
28 addr= (void **) alloca(alloca_size);
29
30=== modified file 'mysys/my_thr_init.c'
31--- mysys/my_thr_init.c 2014-04-23 09:46:00 +0000
32+++ mysys/my_thr_init.c 2014-12-02 08:26:06 +0000
33@@ -344,8 +344,34 @@
34 mysql_mutex_init(key_my_thread_var_mutex, &tmp->mutex, MY_MUTEX_INIT_FAST);
35 mysql_cond_init(key_my_thread_var_suspend, &tmp->suspend, NULL);
36
37- tmp->stack_ends_here= (char*)&tmp +
38- STACK_DIRECTION * (long)my_thread_stack_size;
39+#if defined(_GNU_SOURCE) && (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600)
40+ {
41+ pthread_attr_t attr;
42+ char* stack_addr;
43+ size_t stack_size;
44+
45+ if (pthread_attr_init(&attr) ||
46+ pthread_getattr_np(tmp->pthread_self, &attr) ||
47+ pthread_attr_getstack(&attr, (void**)&stack_addr, &stack_size))
48+ {
49+ error= 1;
50+ goto end;
51+ }
52+ tmp->stack_ends_here= stack_addr;
53+
54+ if (pthread_attr_destroy(&attr))
55+ {
56+ error= 1;
57+ goto end;
58+ }
59+ }
60+#else //defined(_GNU_SOURCE) && (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600)
61+ /* assuming tmp is located within the first page of the stack */
62+ tmp->stack_ends_here= (void **)(((long long)&tmp & (~((long long)my_getpagesize() - 1))) +
63+ ((STACK_DIRECTION == 1) ?
64+ my_thread_stack_size :
65+ my_getpagesize() - my_thread_stack_size));
66+#endif //defined(_GNU_SOURCE) && (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600)
67
68 mysql_mutex_lock(&THR_LOCK_threads);
69 tmp->id= ++thread_id;

Subscribers

People subscribed via source and target branches