[Windows] _thread.start_new_thread() should close the thread handle #88602

Activity

1. vstinner commented on Jun 16, 2021

   

   vstinner

   on Jun 16, 2021

   MemberAuthor

   More actions

   _thread.start_new_thread() is implemented by calling _beginthreadex().

   Currently, when the thread completes: PyThread_exit_thread() is called which calls "_endthreadex(0)" on Windows. I proposed to no longer call it explicitly in bpo-44434.

   _endthreadex() documentation says that the thread handle must be closed explicitly (with CloseHandle()), same applies for ExitThread().

   "Unlike _endthread, the _endthreadex function does not close the thread handle, thereby allowing other threads to block on this one without fear that the handle will be freed out from under the system."

   _endthread() closes the thread handle, but Python uses _endthreadex().

   Should Python be modified to close explicitly the thread handle once the thread terminated?

   _endthread() and _endthreadex() documentation:
   https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/endthread-endthreadex?view=msvc-160

   Example closing the thread handle in the thread which created the thread:
   https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-160

   Simplified code:
   ---

       hThread = (HANDLE)_beginthreadex( NULL, 0, &SecondThreadFunc, NULL, 0, &threadID );
       WaitForSingleObject( hThread, INFINITE );
       CloseHandle( hThread );

   ---

   Would it be safe to close the handle just after PyThread_start_new_thread() success?

   Or should it be done in the C function thread_run(), just before existing, CloseHandle(GetCurrentThread())?

   To debug this issue, GetHandleInformation() can be used to check if the handle is still open or not. For example, call os.get_handle_inheritable(handle) in Python.
2. vstinner

   added

   3.11only security fixesonly security fixes

   OS-windows

   on Jun 16, 2021
3. eryksun commented on Jun 16, 2021

   

   eryksun

   on Jun 16, 2021

   Contributor

   More actions

   Would it be safe to close the handle just after PyThread_start_new_thread()
   success?

   We already close the handle in PyThread_start_new_thread() in Python/thread_nt.h:

       if (hThread == 0) {
           // ...
       }
       else {
           dprintf(("%lu: PyThread_start_new_thread succeeded: %p\n",
                    PyThread_get_thread_ident(), (void*)hThread));
           CloseHandle(hThread);
       }
4. vstinner commented on Jun 16, 2021

   

   vstinner

   on Jun 16, 2021

   MemberAuthor

   More actions

   CloseHandle(GetCurrentThread())

   This is useless. GetCurrentThread() returns a pseudo handle. The GetCurrentThread() documentation says:

   "The pseudo handle need not be closed when it is no longer needed. Calling the CloseHandle function with this handle has no effect."
5. vstinner commented on Jun 16, 2021

   

   vstinner

   on Jun 16, 2021

   MemberAuthor

   More actions

   [Windows] _thread.start_new_thread() should close the thread handle

   So far, I'm not convinced that Python must be changed.

   I modified my Python locally so thread_run() writes GetCurrentThread() to stderr.

   Example:
   ---

   SLEEP = 5
   
   import ctypes
   import _thread
   import time
   import os
   
   HANDLE = ctypes.c_voidp
   DWORD = ctypes.c_ulong
   
   ResumeThread = ctypes.windll.kernel32.ResumeThread
   ResumeThread.argtypes = (HANDLE,)
   ResumeThread.restype = DWORD
   
   SuspendThread = ctypes.windll.kernel32.SuspendThread
   SuspendThread.argtypes = (HANDLE,)
   SuspendThread.restype = DWORD
   
   def func():
       time.sleep(SLEEP)
       print("--thread exit--")
   
   def check_handle(handle):
       x = SuspendThread(handle)
       print(f"SuspendThread({handle}) -> {x}")
       x = ResumeThread(handle)
       print(f"ResumeThread({handle}) -> {x}")
   
   handle = _thread.start_new_thread(func, ())
   print(f"start_new_thread() -> {handle}")
   check_handle(handle)
   time.sleep(SLEEP+1)
   check_handle(handle)

   ---

   Output:
   ---
   start_new_thread() -> 2436
   SuspendThread(2436) -> 4294967295
   ResumeThread(2436) -> 4294967295
   thread_run: GetCurrentThread() -> -2
   --thread exit--
   SuspendThread(2436) -> 4294967295
   ResumeThread(2436) -> 4294967295
   ---

   It seems like the handle returned by _beginthreadex() cannot be used with SuspendThread() or ResumeThread(): both fail. GetHandleInformation() also fails on this handle (I tried os.get_handle_inheritable()).
6. vstinner commented on Jun 16, 2021

   

   vstinner

   on Jun 16, 2021

   MemberAuthor

   More actions

   Eryk:

   We already close the handle in PyThread_start_new_thread() in Python/thread_nt.h

   Oh right! I completely missed this call. Thanks for double checking the issue ;-) I close the issue as "not a bug".
7. vstinner

   closed this as completedon Jun 16, 2021
8. vstinner

   added

   invalid

   on Jun 16, 2021
9. vstinner

   closed this as completedon Jun 16, 2021
10. vstinner

    added

    invalid

    on Jun 16, 2021
11. ezio-melotti

    transferred this issue fromon Apr 10, 2022