Bug 19511 - 8MB memory leak in pthread_create in case of failure when non-root user changes priority
Summary: 8MB memory leak in pthread_create in case of failure when non-root user chang...
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: nptl (show other bugs)
Version: 2.22
: P3 normal
Target Milestone: 2.34
Assignee: Adhemerval Zanella
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-22 03:09 UTC by yutao
Modified: 2021-06-09 18:17 UTC (History)
5 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
pthread_test.c sample code (915 bytes, text/x-csrc)
2016-01-22 03:09 UTC, yutao
Details
non_root.log (234 bytes, text/x-log)
2016-01-22 03:10 UTC, yutao
Details
root.log (184 bytes, text/x-log)
2016-01-22 03:11 UTC, yutao
Details

Note You need to log in before you can comment on or make changes to this bug.
Description yutao 2016-01-22 03:09:31 UTC
Created attachment 8914 [details]
pthread_test.c sample code

1. description
For non-root user, when changes priority of thread, pthread_create will fail, and there is 8MB stack memory leak;
For root user, there is no such issue;

2. sample code
pthread_test.c
gcc pthread_test.c -lpthread

use SHOW_VMSIZE, ReadProcStatusAndGetFieldAsSizeT to show the VmSize usage at run time;

3. log
non_root.log:
When run it with non-root user, VmSize will keep increasing
(you could run it several times to check the result, or you could change the loop to 10000 to check it)
root.log:
When run it with root user, VmSize is stable

4. tested glibc
glibc 2.21, 2.22

5. environment
$ uname -a
3.19.0-43-generic #49-Ubuntu SMP Sun Dec 27 19:43:07 UTC 2015 x86_64 x86_64 x86_64 GNU/Linux
$ cat /etc/issue
Ubuntu 15.04
Comment 1 yutao 2016-01-22 03:10:39 UTC
Created attachment 8915 [details]
non_root.log
Comment 2 yutao 2016-01-22 03:11:09 UTC
Created attachment 8916 [details]
root.log
Comment 3 Florian Weimer 2016-01-25 15:17:50 UTC
What happens is that the thread created “stopped” (so that the priorities can be applied before the thread function is started), but we fail to take into account that the thread could be canceled in the stopped phase.  Then the thread will never be joined, so its thread stack is never flagged for re-use.

A fix could perhaps look like this, but the existing cancellation handling looks racy: What happens if SIGCANCEL arrives before the handler is set up?  Can SIGCANCEL be lost?

diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 5216041..69e5bc6 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -308,6 +308,7 @@ START_THREAD_DEFN
   unwind_buf.priv.data.cleanup = NULL;
 
   int not_first_call;
+  bool start_routine_called = false;
   not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf);
   if (__glibc_likely (! not_first_call))
     {
@@ -329,6 +330,7 @@ START_THREAD_DEFN
       LIBC_PROBE (pthread_start, 3, (pthread_t) pd, pd->start_routine, pd->arg);
 
       /* Run the code the user provided.  */
+      start_routine_called = true;
 #ifdef CALL_THREAD_FCT
       THREAD_SETMEM (pd, result, CALL_THREAD_FCT (pd));
 #else
@@ -435,7 +437,7 @@ START_THREAD_DEFN
     __madvise (pd->stackblock, freesize - PTHREAD_STACK_MIN, MADV_DONTNEED);
 
   /* If the thread is detached free the TCB.  */
-  if (IS_DETACHED (pd))
+  if (IS_DETACHED (pd) || !start_routine_called)
     /* Free the TCB.  */
     __free_tcb (pd);
   else if (__glibc_unlikely (pd->cancelhandling & SETXID_BITMASK))
Comment 4 Carlos O'Donell 2017-01-24 04:19:31 UTC
SIGCANCEL cannot be lost. The parent in __pthread_initialize_minimal_internal will setup the handler long before any pthread_create code is run.

I think the simplest solution is actually to make the parent responsible for the cleanup since it is the owner of the PD after create_thread returns an error.

So the parent should run pthread_join on PD to reap the stack.

e.g.

682   if (__glibc_unlikely (retval != 0))
683     {
684       /* If thread creation "failed", that might mean that the thread got
685          created and ran a little--short of running user code--but then
686          create_thread cancelled it.  In that case, the thread will do all
687          its own cleanup just like a normal thread exit after a successful
688          creation would do.  */
689 
690       if (thread_ran)
691         assert (pd->stopped_start);

If thread_ran, and !detached, then stopped_start == true, so the user code can't have run and detached itself, so we are still the owner of PD and can issue a pthread_join to reap the thread.
Comment 5 Carlos O'Donell 2017-01-24 04:20:45 UTC
I discovered this bug, and then this issue while auditing the code for bug 20116.
Comment 6 Adhemerval Zanella 2021-06-09 18:17:49 UTC
Fixed on 2.34 (02189e8fb00c3c7f4e67476e21011a22c5dee707).