python / python/cpython

LIST_APPEND and SET_ADD opcodes safety in Free Threading

Open
#152,288 5 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

topic-free-threading
Dominant language
Python
Stars
77.2k
Forks
35.9k
PR merge metrics
PR metrics pending

Description

Hi,

While reviewing PR gh-152273, I moved _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(so); check to set_add_entry_takeref() to check for bugs. I discovered that SET_ADD opcode calls _PySet_AddTakeRef() without getting the set critical section. That's surprising and IMO it would deserve adding a comment to explain why it's safe to omit locking here.

LIST_APPEND has a similar design (also omit locking).


The following code uses the evil gc.get_objects() function to call set.add() in a different thread while the main thread is building a set:

import dis
import gc
import threading

GLOBAL_SET = None

def worker():
    EVENT.wait()
    obj = GLOBAL_SET
    for i in range(100):
        # Call set.add() which uses the critical section
        obj.add(i)


MARKER = b"MARKER".decode()
EVENT = threading.Event()

class EvilHash:
    def __init__(self, hash_value):
        self.hash_value = hash_value

    def __hash__(self):
        global GLOBAL_SET, EVENT
        if GLOBAL_SET is None:
            # Invoke the evil gc.get_objects()!
            for obj in gc.get_objects():
                if isinstance(obj, set) and MARKER in obj:
                    GLOBAL_SET = obj
                    EVENT.set()
        return self.hash_value

    def __repr__(self):
        return f"EvilHash({self.hash_value})"

def func():
    build_set = {
        # Use *list so following items are added by SET_ADD opcode
        *[MARKER],
        # Added by SET_ADD which calls _PySet_AddTakeRef()
        # without the critical section
        EvilHash(0), EvilHash(1), EvilHash(2), EvilHash(3), EvilHash(4),
        EvilHash(5)}

    print(build_set)
    print("Length:", len(build_set))
    if len(build_set) != 107:
        raise Exception("race condition!")

thread = threading.Thread(target=worker)
thread.start()
func()
thread.join()
#dis.dis(func)

I expected the code to fail randomly, but so far I failed to trigger a race condition on Free Threading. Running the code on Python built with --with-thread-sanitizer doesn't show any warning.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by tracing SET_ADD and LIST_APPEND through _PySet_AddTakeRef() and set_add_entry_takeref(), using the provided threaded reproducer as the behavioral check. Run it on a free-threaded build, including the thread-sanitized build, and determine whether omitting the critical section is safe. Done means the safety rationale is established and any necessary explanation or correction is covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.