Fix issue 18063 - thread_attachThis returns dangling pointer

src/core/thread/osthread.d

@@ -216,6 +216,7 @@ class Thread : ThreadBase
     version (Posix)
     {
         private shared bool     m_isRunning;
+        private bool            m_isOwned;
     }
 
     version (Darwin)
@@ -299,9 +300,12 @@ class Thread : ThreadBase
         }
         else version (Posix)
         {
-            if (m_addr != m_addr.init)
-                pthread_detach( m_addr );
-            m_addr = m_addr.init;
+            if (m_isOwned)
+            {
+                if (m_addr != m_addr.init)
+                    pthread_detach( m_addr );
+                m_addr = m_addr.init;
+            }
         }
         version (Darwin)
         {
@@ -492,6 +496,7 @@ class Thread : ThreadBase
                 }
                 if ( pthread_attr_destroy( &attr ) != 0 )
                     onThreadError( "Error destroying thread attributes" );
+                m_isOwned = true;
             }
             version (Darwin)
             {
@@ -1035,6 +1040,20 @@ unittest
 }
 
 
+unittest
+{
+    auto t = new Thread(
+    {
+        auto old = Thread.getThis();
+        assert(old !is null);
+        thread_setThis(null);
+        assert(Thread.getThis() is null);
+        thread_setThis(old);
+    }).start;
+    t.join();
+}
+
+
 unittest
 {
     enum MSG = "Test message.";
@@ -1248,6 +1267,7 @@ private extern (D) ThreadBase attachThread(ThreadBase _thisThread) @nogc nothrow
         thisContext.tstack = thisContext.bstack;
 
         atomicStore!(MemoryOrder.raw)(thisThread.toThread.m_isRunning, true);
+        thisThread.m_isOwned = false;
     }
     thisThread.m_isDaemon = true;
     thisThread.tlsGCdataInit();
@@ -1267,8 +1287,16 @@ private extern (D) ThreadBase attachThread(ThreadBase _thisThread) @nogc nothrow
 }
 
 /**
- * Registers the calling thread for use with the D Runtime.  If this routine
- * is called for a thread which is already registered, no action is performed.
+ * Registers the calling thread for use with the D Runtime.  If this routine is
+ * called for a thread which is already registered, no action is performed.  On
+ * Posix systems, the D Runtime does not take ownership of the thread;
+ * specifically, it does not call `pthread_detach` during cleanup.
+ *
+ * Threads registered by this routine should normally be deregistered by
+ * `thread_detachThis`.  Although `thread_detachByAddr` and
+ * `thread_detachInstance` can be used as well, such threads cannot be registered
+ * again by `thread_attachThis` unless `thread_setThis` is called with the
+ * `null` reference first.
  *
  * NOTE: This routine does not run thread-local static constructors when called.
  *       If full functionality as a D thread is desired, the following function

src/core/thread/threadbase.d

@@ -798,6 +798,12 @@ extern (C) bool thread_isMainThread() nothrow @nogc
  * Registers the calling thread for use with the D Runtime.  If this routine
  * is called for a thread which is already registered, no action is performed.
  *
+ * Threads registered by this routine should normally be deregistered by $(D
+ * thread_detachThis).  Although $(D thread_detachByAddr) and $(D
+ * thread_detachInstance) can be used as well, such threads cannot be registered
+ * again by $(D thread_attachThis) unless $(D thread_setThis) is called with the
+ * $(D null) value first.
+ *
  * NOTE: This routine does not run thread-local static constructors when called.
  *       If full functionality as a D thread is desired, the following function
  *       must be called after thread_attachThis:
@@ -826,8 +832,14 @@ package ThreadT thread_attachThis_tpl(ThreadT)()
  */
 extern (C) void thread_detachThis() nothrow @nogc
 {
+    ThreadBase.slock.lock_nothrow();
+    scope(exit) ThreadBase.slock.unlock_nothrow();
+
     if (auto t = ThreadBase.getThis())
+    {
         ThreadBase.remove(t);
+        t.setThis(null);
+    }
 }
 
 
@@ -844,6 +856,9 @@ extern (C) void thread_detachThis() nothrow @nogc
  */
 extern (C) void thread_detachByAddr(ThreadID addr)
 {
+    ThreadBase.slock.lock_nothrow();
+    scope(exit) ThreadBase.slock.unlock_nothrow();
+
     if (auto t = thread_findByAddr(addr))
         ThreadBase.remove(t);
 }
@@ -852,6 +867,9 @@ extern (C) void thread_detachByAddr(ThreadID addr)
 /// ditto
 extern (C) void thread_detachInstance(ThreadBase t) nothrow @nogc
 {
+    ThreadBase.slock.lock_nothrow();
+    scope(exit) ThreadBase.slock.unlock_nothrow();
+
     ThreadBase.remove(t);
 }
 

test/thread/Makefile

@@ -1,6 +1,6 @@
 include ../common.mak
 
-TESTS:=fiber_guard_page external_threads tlsgc_sections test_import tlsstack join_detach
+TESTS:=fiber_guard_page external_threads tlsgc_sections test_import tlsstack join_detach attach_detach
 
 .PHONY: all clean
 all: $(addprefix $(ROOT)/,$(addsuffix .done,$(TESTS)))
@@ -34,6 +34,11 @@ $(ROOT)/tlsstack.done: $(ROOT)/%.done : $(ROOT)/%
 $(ROOT)/join_detach.done: $(ROOT)/%.done : $(ROOT)/%
 	@echo Testing $* is currently disabled!
 
+$(ROOT)/attach_detach.done: $(ROOT)/%.done : $(ROOT)/%
+	@echo Testing $*
+	$(QUIET)$(TIMELIMIT)$(ROOT)/$*
+	@touch $@
+
 $(ROOT)/%: $(SRC)/%.d
 	$(QUIET)$(DMD) $(DFLAGS) -of$@ $<
 

test/thread/src/attach_detach.d

@@ -0,0 +1,160 @@
+version (Posix)
+{
+
+import core.thread;
+import core.sys.posix.pthread;
+import core.stdc.stdlib;
+import core.stdc.time;
+
+// This program creates threads that are started outside of D runtime and
+// stresses attaching and detaching those threads to the D runtime.
+
+struct MyThread
+{
+    pthread_t t;
+    bool stopRequested;
+}
+
+enum totalThreads = 4;
+
+enum runTimeSeconds = 5;    // Must be less than timelimit's
+MyThread[totalThreads] threads;
+
+auto exerciseGC() {
+    int[] arr;
+    foreach (i; 0 .. 1000)
+        arr ~= i;
+    return arr;
+}
+
+// This represents an API function of a non-D framework. Since we don't have any
+// control on the lifetime of this thread, we have to attach upon entry and
+// detach upon exit.
+void api_foo()
+{
+    auto t = thread_attachThis();
+    scope(exit)
+    {
+        // Pick a detachment method
+        final switch (rand() % 3)
+        {
+        case 0:
+            thread_detachThis();
+            break;
+        case 1:
+            thread_detachByAddr(t.id);
+            // thread_setThis must be called by the detached thread; it happens
+            // to be the case in this test.
+            thread_setThis(null);
+            break;
+        case 2:
+            thread_detachInstance(t);
+            // thread_setThis must be called by the detached thread; it happens
+            // to be the case in this test.
+            thread_setThis(null);
+            break;
+        }
+    }
+
+    assert_thread_is_attached(t.id);
+    cast(void)exerciseGC();
+}
+
+// Make calls to an api function and exit when requested
+extern(C) void * thread_func(void * arg)
+{
+    MyThread *t = cast(MyThread*)arg;
+
+    while (!t.stopRequested)
+        api_foo();
+
+    return arg;
+}
+
+void start_thread(ref MyThread t)
+{
+    pthread_attr_t attr;
+    int err = pthread_attr_init(&attr);
+    assert(!err);
+
+    t.stopRequested = false;
+    err = pthread_create(&t.t, &attr, &thread_func, cast(void*)&t);
+    assert(!err);
+
+    err = pthread_attr_destroy(&attr);
+    assert(!err);
+}
+
+void start_threads()
+{
+    foreach (ref t; threads)
+        start_thread(t);
+}
+
+void stop_thread(ref MyThread t)
+{
+    t.stopRequested = true;
+    const err = pthread_join(t.t, null);
+    assert(!err);
+
+    assert_thread_is_gone(t.t);
+}
+
+void stop_threads()
+{
+    foreach (ref t; threads)
+        stop_thread(t);
+}
+
+void assert_thread_is_attached(pthread_t tid)
+{
+    size_t found = 0;
+    foreach (t; Thread.getAll())
+        if (tid == t.id)
+        {
+            ++found;
+        }
+    assert(found == 1);
+}
+
+void assert_thread_is_gone(pthread_t tid)
+{
+    foreach (t; Thread.getAll())
+        assert(tid != t.id);
+}
+
+// Occasionally stop threads and start new ones
+void watch_threads()
+{
+    const start = time(null);
+
+    while ((time(null) - start) < runTimeSeconds)
+    {
+        foreach (ref t; threads)
+        {
+            const shouldStop = ((rand() % 100) == 0);
+            if (shouldStop)
+            {
+                stop_thread(t);
+                start_thread(t);
+            }
+        }
+    }
+}
+
+void main()
+{
+    start_threads();
+    watch_threads();
+    stop_threads();
+}
+
+} // version (Posix)
+else
+{
+
+void main()
+{
+}
+
+}