LIST_APPEND and SET_ADD opcodes safety in Free Threading

[vstinner]

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.

[corona10]

cc @colesbury

[vstinner]

I created this issue while working on issues about immutable types (frozendict and frozenset) being exposed by the GC while building new collections:

• frozendict issue: Data race on frozendict reads during construction in the free-threading build #151722
• frozenset issue: Need to defer GC tracking of frozenset/set construction #152235

[vstinner]

If we care about fixing the evil case using gc.get_objects() to be 100% safe, the following bytecode:

BUILD_SET
SET_UPDATE
SET_ADD
...
SET_ADD

should use new (hypothetical) UNTRACKED and GC_TRACK opcodes. Something like:

BUILD_SET_UNTRACKED
SET_UPDATE
SET_ADD
...
SET_ADD
GC_TRACK

[kumaraditya303]

FWIW I disagree that this needs to be fixed at all. get_objects is a problematic API and should be used for debugging only in specific cases. get_objects exposes all GC objects and it breaks code which relies on check if object has 1 refcnt and mutate it which is a well known optimization technique used in C code.

I'd like to know the use-case of why fixing this is important otherwise I am strong -1 on this and similar changes.

[vstinner]

Well, at the beginning, I just asked for adding a comment on LIST_APPEND and SET_ADD opcodes to explain why it's safe to omit locking here :-) I'm fine with not fixing the corner case of gc.get_objects().

gc.get_referrers() documentation has this warning:

Care must be taken when using objects returned by get_referrers() because some of them could still be under construction and hence in a temporarily invalid state. Avoid using get_referrers() for any purpose other than debugging.