Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
vstinner opened this issue Jun 16, 2021 · 5 comments
Closed

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

vstinner opened this issue Jun 16, 2021 · 5 comments
Labels
3.11 only security fixes OS-windows

Comments

@vstinner
Copy link
Member

BPO 44436
Nosy @pfmoore, @vstinner, @tjguk, @zware, @eryksun, @zooba

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2021-06-16.16:03:07.236>
created_at = <Date 2021-06-16.15:43:23.535>
labels = ['invalid', 'OS-windows', '3.11']
title = '[Windows] _thread.start_new_thread() should close the thread handle'
updated_at = <Date 2021-06-16.16:03:07.236>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2021-06-16.16:03:07.236>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2021-06-16.16:03:07.236>
closer = 'vstinner'
components = ['Windows']
creation = <Date 2021-06-16.15:43:23.535>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 44436
keywords = []
message_count = 5.0
messages = ['395939', '395944', '395945', '395947', '395948']
nosy_count = 6.0
nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue44436'
versions = ['Python 3.11']

@vstinner
Copy link
Member Author

_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.

@vstinner vstinner added 3.11 only security fixes OS-windows labels Jun 16, 2021
@eryksun
Copy link
Contributor

eryksun commented Jun 16, 2021

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);
    }

@vstinner
Copy link
Member Author

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."

@vstinner
Copy link
Member Author

[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()).

@vstinner
Copy link
Member Author

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".

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes OS-windows
Projects
None yet
Development

No branches or pull requests

2 participants