[ofw] [bug] [COMPLIB] A bug in COMPLIB thread handling mechanism.

[Smith, Stan]

OK, thanks. My eyes missed the & p_thread->osd.p_thread.
Looks fine to commit sans Fab's overall comment.

Stan.

From: Leonid Keller [mailto:leonid at mellanox.co.il]
Sent: Tuesday, October 18, 2011 12:54 AM
To: Smith, Stan; ofw_list
Subject: RE: [ofw] [bug] [COMPLIB] A bug in COMPLIB thread handling mechanism.

PSB

From: Smith, Stan [mailto:stan.smith at intel.com]
Sent: Monday, October 17, 2011 8:15 PM
To: Leonid Keller; ofw_list
Subject: RE: [ofw] [bug] [COMPLIB] A bug in COMPLIB thread handling mechanism.

Hi Leo,
  I tested your patch with no problems. Although I did have one question; please see below.

Stan.


From: ofw-bounces at lists.openfabrics.org [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Leonid Keller
Sent: Sunday, October 09, 2011 4:26 AM
To: ofw_list
Subject: [ofw] [bug] [COMPLIB] A bug in COMPLIB thread handling mechanism.

We got a BSOD in cl_thread_destroy() function on ZwClose().

The code review showed that the old mechanism doesn't take a reference on the created thread and waits for the thread exit while it may already be a non-existed object.
It also closes the thread handle too late.
Please, review the patch.


Index: B:/users/leonid/svn/winib/trunk/core/complib/kernel/cl_thread.c
===================================================================
--- B:/users/leonid/svn/winib/trunk/core/complib/kernel/cl_thread.c   (revision 8921)
+++ B:/users/leonid/svn/winib/trunk/core/complib/kernel/cl_thread.c                (revision 8922)
@@ -38,11 +38,8 @@
 __thread_callback(
                IN           cl_thread_t* p_thread )
 {
-              /* Store the thread pointer so that destroy and is_current_thread work. */
-              p_thread->osd.p_thread = KeGetCurrentThread();
-

If p_thread->osd.p_thread initialization is removed, where is it set as it is dereferenced later on?             <==== QUESTION?
[LK] In ObReferenceObjectByHandle() below. It takes a reference.


                /* Bump the thread's priority. */
-              KeSetPriorityThread( p_thread->osd.p_thread, LOW_REALTIME_PRIORITY );
+             KeSetPriorityThread( KeGetCurrentThread(), LOW_REALTIME_PRIORITY );

                /* Call the user's thread function. */
                (*p_thread->pfn_callback)( (void*)p_thread->context );
@@ -91,6 +88,15 @@
                if( !NT_SUCCESS( status ) )
                                return( CL_ERROR );

+             /* get pointer to thread object to wait on it's exit */
+             status = ObReferenceObjectByHandle( p_thread->osd.h_thread, THREAD_ALL_ACCESS,
+                             NULL, KernelMode, (PVOID*)&p_thread->osd.p_thread, NULL );
+    CL_ASSERT(status == STATUS_SUCCESS); // According to MSDN, must succeed if I set the params
+
+             /* Close the handle to the thread. */
+             status = ZwClose( p_thread->osd.h_thread );
+    CL_ASSERT(NT_SUCCESS(status)); // Should always succeed
+
                return( CL_SUCCESS );
 }

@@ -112,8 +118,8 @@
                KeWaitForSingleObject( p_thread->osd.p_thread, Executive, KernelMode,
                                FALSE, NULL );

-              /* Close the handle to the thread. */
-              ZwClose( p_thread->osd.h_thread );
+             /* Release the reference to thread object */
+             ObDereferenceObject( p_thread->osd.p_thread );

                /*
                 * Reset the handle in case the user calls destroy and the thread is
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openfabrics.org/pipermail/ofw/attachments/20111018/f47fedb3/attachment.html>