Re: bug in libpthreads

[Thomas Schwinge]

Hi!

On Thu, 10 May 2012 17:38:26 -0700 (PDT), Thomas Thomas <ricinwich@yahoo.com> 
wrote:
> In function initialize_pthread in file pthread/pt-alloc.c, you need to
> initialize the value of have_kernel_resources to zero if the thread
> structure is not recycled. Following the logic of
> __pthread_create_internal: we go from __pthread_alloc to
> __pthread_thread_alloc without touching have_kernel_resources.
> 
> Then, __pthread_thread_alloc exits immediately if
> have_kernel_resources is non-zero: not good if we haven't
> actually allocated these resources.
> 
> Sorry this isn't a patch, but it is only a one line fix,

In the t/fix_have_kernel_resources branch, I have a preliminary patch
(from April 2009...) for that issue, inlined below; would you like to
pursue that issue further?

Subject: [PATCH, UNFINISHED] Fix handling of have_kernel_resources on Mach/Hurd.

From: Thomas Schwinge <thomas@schwinge.name>

<http://lists.gnu.org/archive/html/bug-hurd/2009-04/msg00028.html>

Further comments by Neal:

<neal> do you want a reply on the libpthread one inline?
<neal> the short answer is: yes, that's a bug
<neal> unfortunately, your fix is not enough
<neal> the predicate controls two resources: the wakeup port and the thread
  itself
<tschwinge> Oh, right, I see.
<neal> also, there may be a race:
<neal> set the predicate to free, then kill the thread
<neal> that's not so good
<neal> so a proper solution requires a bit more thought
<tschwinge> I think I wondered about that as well.  But isn't there the same
  problem with Viengoos?
<neal> it is difficult as cleanly committing suicide is hard :-)
<neal> could be
<neal> on viengoos, I don't actually deallocate the thread in pt-thread-halt.c
<neal> I just call suspend
<neal> the thread is only deallocated in pt-thread-dealloc.c

---
 sysdeps/mach/hurd/pt-init-specific.c |   29 +++++++++++++++++++++++++++++
 sysdeps/mach/pt-thread-dealloc.c     |    2 ++
 sysdeps/mach/pt-thread-halt.c        |   19 +++++++++++++++++--
 3 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/sysdeps/mach/hurd/pt-init-specific.c 
b/sysdeps/mach/hurd/pt-init-specific.c
new file mode 100644
index 0000000..506527d
--- /dev/null
+++ b/sysdeps/mach/hurd/pt-init-specific.c
@@ -0,0 +1,29 @@
+/* __pthread_init_specific.  Hurd on Mach version.
+   Copyright (C) 2002, 2009 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Library General Public License as
+   published by the Free Software Foundation; either version 2 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Library General Public License for more details.
+
+   You should have received a copy of the GNU Library General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If not,
+   write to the Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#include <pthread.h>
+#include <pt-internal.h>
+
+error_t
+__pthread_init_specific (struct __pthread *thread)
+{
+  thread->thread_specifics = 0;
+  thread->have_kernel_resources = 0;
+  return 0;
+}
diff --git a/sysdeps/mach/pt-thread-dealloc.c b/sysdeps/mach/pt-thread-dealloc.c
index 55d8c4d..0c4a4fc 100644
--- a/sysdeps/mach/pt-thread-dealloc.c
+++ b/sysdeps/mach/pt-thread-dealloc.c
@@ -38,4 +38,6 @@ __pthread_thread_dealloc (struct __pthread *thread)
      assert.  */
   __mach_port_destroy (__mach_task_self (),
                       thread->wakeupmsg.msgh_remote_port);
+
+  thread->have_kernel_resources = 0;
 }
diff --git a/sysdeps/mach/pt-thread-halt.c b/sysdeps/mach/pt-thread-halt.c
index 973cde1..a9c3858 100644
--- a/sysdeps/mach/pt-thread-halt.c
+++ b/sysdeps/mach/pt-thread-halt.c
@@ -32,6 +32,21 @@
 void
 __pthread_thread_halt (struct __pthread *thread)
 {
-  error_t err = __thread_terminate (thread->kernel_thread);
-  assert_perror (err);
+  if (thread->have_kernel_resources)
+    {
+      if (thread == _pthread_self ())
+       {
+         while (1)
+           {
+             error_t err = __thread_suspend (thread->kernel_thread);
+             assert_perror (err);
+             assert (! "Failed to suspend self.");
+           }
+       }
+      else
+       {
+         error_t err = __thread_terminate (thread->kernel_thread);
+         assert_perror (err);
+       }
+    }
 }
-- 
tg: (081c7ae..) t/fix_have_kernel_resources (depends on: master)


Grüße,
 Thomas

pgp_fCa7ZdYUb.pgp
Description: PGP signature