Active thread list may be inaccurate due to data type mismatch

[vfazio]

Bug report

Bug description:

0e9c364 changed thread_get_ident to convert a unsigned long long vs the previous unsigned long.

static PyObject *
thread_get_ident(PyObject *self, PyObject *Py_UNUSED(ignored))
{
    PyThread_ident_t ident = PyThread_get_thread_ident_ex();  // <-- ULL
    if (ident == PYTHREAD_INVALID_THREAD_ID) {
        PyErr_SetString(ThreadError, "no current thread ident");
        return NULL;
    }
    return PyLong_FromUnsignedLongLong(ident);
}

However, after #114839 commit 76bde03

MainThread is now a special case because it doesn't use self._set_ident():

class _MainThread(Thread):

    def __init__(self):
        Thread.__init__(self, name="MainThread", daemon=False)
        self._started.set()
        self._ident = _get_main_thread_ident()
        self._handle = _make_thread_handle(self._ident)
        if _HAVE_THREAD_NATIVE_ID:
            self._set_native_id()
        with _active_limbo_lock:
            _active[self._ident] = self

It inserts an identifier from a special function which is always the clipped unsigned long from the runtime struct into the active thread list.

static PyObject *
thread__get_main_thread_ident(PyObject *module, PyObject *Py_UNUSED(ignored))
{
    return PyLong_FromUnsignedLongLong(_PyRuntime.main_thread);
}

    /* Platform-specific identifier and PyThreadState, respectively, for the
       main thread in the main interpreter. */
    unsigned long main_thread;

    // Set it to the ID of the main thread of the main interpreter.
    runtime->main_thread = PyThread_get_thread_ident();

Because of this, on some platforms/libc implementations, we can observe a failure to look up the current thread because of the mismatch between clipped UL value vs the expected ULL value:

>>> import threading
>>> ct = threading.current_thread()
>>> ct
<_DummyThread(Dummy-1, started daemon 18446744072483979068)>
>>> hex(ct.ident)
'0xffffffffb6f33f3c'
>>> main = threading.main_thread()
>>> hex(main.ident)
'0xb6f33f3c'
>>> main._set_ident()
>>> hex(main.ident)
'0xffffffffb6f33f3c'

def current_thread():
    """Return the current Thread object, corresponding to the caller's thread of control.

    If the caller's thread of control was not created through the threading
    module, a dummy thread object with limited functionality is returned.

    """
    try:
        return _active[get_ident()]
    except KeyError:
        return _DummyThread()

Should main_thread to be a PyThread_ident_t ? or should MainThread continue to call _set_ident()?

CPython versions tested on:

3.13

Operating systems tested on:

Linux

Linked PRs

• gh-130115: fix thread identifiers for 32-bit musl #130391
• [3.13] gh-130115: fix thread identifiers for 32-bit musl (GH-130391) #132089

[vfazio]

@pitrou @mpage FYI

[vfazio]

Note this was on MUSL libc on 32bit arm.

>>> os.uname()
posix.uname_result(sysname='Linux', nodename='buildroot', release='5.10.202', version='#1 Wed Dec 6 01:22:06 CET 2023', machine='armv5tejl')

This was discovered by some code that is relying on deprecated asyncio behavior for get_event_loop to automatically start a new event loop:

cpython/Lib/asyncio/events.py

Line 695 in 9e23e0a

threading.current_thread() is threading.main_thread()):

This check fails in this case since the thread can't be found and a DummyThread is created.

There may be wider impacts in asyncio since there are a number of conditions predicated on the current_thread() is main_thread() check working.

[mpage]

Thanks for the detailed bug report! I can repro this using Alpine Linux for armv7 and qemu. I'd lean towards having main_thread be a PyThread_ident_t.

[vfazio]

I haven't looked into the full impacts of that; as in, does it affect any external APIs, etc. If this is the route we want to take, I'm happy to attempt a PR.

I also wasn't sure if there was an explicit reason to avoid the default _set_ident call, I'm sure there is a story there but I didn't get very far before leaving work for vacation.

[mpage]

I also wasn't sure if there was an explicit reason to avoid the default _set_ident call, I'm sure there is a story there but I didn't get very far before leaving work for vacation.

That would set the identity to the identity of the thread that imported the threading module. Most of the time that's the main thread of the main interpreter, but in rare cases (e.g. if it was imported by a thread started using the _thread module) it might not be.

[vfazio]

Thanks, that makes sense. I'll try to putz with this tomorrow and see how far I get.

[vfazio]

At a glance, this change could be viral. This is where it starts to spiral

static PyThreadState *
resolve_final_tstate(_PyRuntimeState *runtime)
{
    PyThreadState *main_tstate = runtime->main_tstate;
    assert(main_tstate != NULL);
    assert(main_tstate->thread_id == runtime->main_thread);

Either we cast it down to UL for thread_id, or we perpetuate the size change everywhere. This has the potential for being as large a change set as aefa7eb

Which then creeps into exposed APIs which have a deprecation period IIUC.

Optionally, we use and maintain a second field runtime->main_thread_ex = PyThread_get_thread_ident_ex() and have:

static PyObject *
thread__get_main_thread_ident(PyObject *module, PyObject *Py_UNUSED(ignored))
{
    return PyLong_FromUnsignedLongLong(_PyRuntime.main_thread_ex);
}

There is one thing still nagging me a bit here and that's the what looks to be sign extension that's occurring on what should presumably be an unsigned type (pthread_t). I should do further tests with the compiler to see what pthread_self is generating. maybe it doesn't matter and we should handle it regardless, but the value looks way off and the 32bit address returned has the high bit set. hopefully there's not a macro or anything accidentally doing a conversion to a signed value.

[vfazio]

pthread_t is not guaranteed to be an arithmetic type:

IEEE Std 1003.1-2001/Cor 2-2004, item XBD/TC2/D6/26 is applied, adding pthread_t to the list
of types that are not required to be arithmetic types, thus allowing pthread_t to be defined as a structure.

It looks like, MUSL defines pthread_t as a struct * in C

https://git.musl-libc.org/cgit/musl/tree/include/alltypes.h.in#n56

#ifdef __cplusplus
TYPEDEF unsigned long pthread_t;
#else
TYPEDEF struct __pthread * pthread_t;
#endif

What I think is happening is the 32 bit pointer is sign extending when cast to ULL.

From GCC's documentation:

https://gcc.gnu.org/onlinedocs/gcc-13.3.0/gcc/Arrays-and-pointers-implementation.html

A cast from integer to pointer discards most-significant bits if the pointer representation is smaller than the
integer type, extends according to the signedness of the integer type if the pointer representation is larger
than the integer type, otherwise the bits are unchanged.

I was messing with www.godbolt.org since i don't have a compiler handy atm

/* Type your code here, or load an example. */
#include <stdio.h>
#include <stdint.h>
#include <inttypes.h>


int main() {
    void * x = (void *)0xb6f33f3c;
    uintptr_t xp = (uintptr_t)0xb6f33f3c;
    unsigned long long thread = (unsigned long long)x;

    printf("%llx\n", (unsigned long long)x);
    printf("%llx\n", (unsigned long long)(uintptr_t)x);
    printf("%llx\n", thread);
    printf("%llx\n", (unsigned long long)xp);
}

Compile on x86_64 with -m32 and we get:

Compiler stderr

<source>: In function 'main':
<source>:10:33: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
   10 |     unsigned long long thread = (unsigned long long)x;
      |                                 ^
<source>:12:22: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
   12 |     printf("%llx\n", (unsigned long long)x);
      |                      ^

Program returned: 0
Program stdout

ffffffffb6f33f3c
b6f33f3c
ffffffffb6f33f3c
b6f33f3c

So when casting to ULL, the 32 bit pointer is sign extended to fill 64bits. When forcefully cast through uintptr_t, the sign bits are dropped.

The question is, how best to handle this. Adding the cast through uintptr_t in PyThread_get_thread_ident_ex could work.

The biggest problem is the mismatch in types across libc headers.

Given how we're using the return value of pthread_self and how pthread_t is documented, it may not be safe to assume that 32bits are good enough to uniquely identify a thread on all implementations, meaning there should probably be follow up tasks on extending the size the thread identifier exposed in public APIs and maybe using a hash map of pthread_t -> some python assigned number within unsigned long limits to avoid API changes.

And I guess we were explicitly warned:

cpython/Python/thread_pthread.h

Line 351 in 359c7dd

- The cast to unsigned long is inherently unsafe.

[vfazio]

This is seems related to some degree to this conversation:

#110848 (comment)

@ericsnowcurrently @gpshead just as an FYI

[mpage]

Thanks for all the digging! Given the blast radius of changing the type _PyRuntimeState::main_thread, I think the best path forward is probably to add a second field with the correct type as you suggested. It's a little ugly, but is hopefully a small and self-contained fix. I'm happy to put up a PR if you don't feel like doing it.

Adding the cast through uintptr_t in PyThread_get_thread_ident_ex could work.

I don't think we want to add a cast through uintptr_t in PyThread_get_thread_ident_ex. We're already on shaky ground from making assumptions about pthread_t, let's not make any more.

meaning there should probably be follow up tasks

Agreed. If we're going to take the time to deal with how we handle pthread_t, I think it would be worthwhile to take a step back and also consider the issues we have with PyThreadStates (e.g. finalization and daemon threads).

on extending the size the thread identifier exposed in public APIs and maybe using a hash map of pthread_t -> some python assigned number within unsigned long limits to avoid API changes

I think there's probably a path that allows us to continue using an unsigned long in public APIs, but transitions internal APIs to use pointers to opaque structs that contain platform-specific data.

[vfazio]

If it's not too much to ask, I'd like to make the PR and maybe have others review? I'm always looking for opportunities to cut my teeth more on CPython in a meaningful way since we use it so heavily where I work; I feel obligated to give back.

Im on a plane tomorrow but may have time to throw this together after I land or the day after. I don't imagine it will take me too long, but I do want to test the patch out in a Buildroot build to make sure it passes muster.

Thanks for taking the time to read through my musings, I think best when I think out loud.