Skip to content

Fix a crash in mpack_encode_list on a FT build - #1252

Open
sobolevn wants to merge 7 commits into
mainfrom
issue-1249
Open

sobolevn wants to merge 7 commits into
mainfrom
issue-1249

Conversation

@sobolevn

Copy link
Copy Markdown
Member

This PR does two things:

  1. It fixes a crash when obj list item was modified by another thread in FT build. obj was used to get the __len__ before the lock. So, the size and contents were not in sync. I fixed this, now we do it under a lock
  2. It adds initial testing infra for such problems which I copied and adapted from CPython

Refs #1249

Comment thread src/msgspec/_core.c
}
status = 0;

cleanup:;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cleanup:;
cleanup:

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, Py_END_CRITICAL_SECTION(); is not a statement :(

Without it:

src/msgspec/_core.c:13049:5: warning: label at end of compound statement is a C23 extension [-Wc23-extensions]
 13049 |     Py_END_CRITICAL_SECTION();
       |     ^
/Users/sobolev/.local/share/uv/python/cpython-3.13.5-macos-aarch64-none/include/python3.13/cpython/critical_section.h:89:5: note: expanded from macro 'Py_END_CRITICAL_SECTION'
   89 |     }
      |     ^
1 warning generated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. A bit confusing to read though I admit 😅
But I guess you get used to it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why Rust and Zig were invented 😆

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your code clear and easy to read?
Then we’re coming for you!
© goto and macros

@Siyet

Siyet commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Moving the size read under the lock fixes the original race, but there is still a crash when another thread modifies a nested list while the outer list is being cleared. I reproduced this on Python 3.14 with the GIL disabled, using the code below without an enc_hook.

CPython can temporarily release the outer list's lock while waiting for the inner list's lock. For example, with inner = [1, 2, 3] and outer = [inner, 99], this sequence is possible:

Thread 1: locks outer and saves len = 2.
Thread 2: modifies inner while holding its lock.
Thread 1: waits for inner's lock, temporarily releasing outer's lock.
Thread 3: clears outer.
Thread 1: resumes encoding with len = 2, although outer is now empty.

The next access to outer through PyList_GET_ITEM can then be out of bounds. The loop also uses element references without owning them across recursive encoding. Please cover this case and protect both the element references and the list accesses. The number of encoded items must also match the length already written in the MessagePack header.

Reproducer for Python 3.14 with the GIL disabled
import threading
import msgspec

inner = [1, 2, 3]
outer = [inner] * 64
stop = threading.Event()

def churn_inner():
    while not stop.is_set():
        inner.append(0)
        inner.pop()

def shrink_outer():
    while not stop.is_set():
        outer.clear()
        outer.extend([inner] * 64)

def encode():
    for _ in range(20000):
        msgspec.msgpack.encode(outer)

mutators = [threading.Thread(target=f)
            for f in (churn_inner, churn_inner, shrink_outer)]
encoders = [threading.Thread(target=encode) for _ in range(4)]
for t in mutators + encoders:
    t.start()
for t in encoders:
    t.join()
stop.set()
for t in mutators:
    t.join()

There is also a slowdown on regular GIL builds: Py_EnterRecursiveCall now runs even for empty lists. In a local macOS microbenchmark, encoding [[] for _ in range(1000)] was about twice as slow as before. Moving the recursion check after the empty-list path removed the slowdown. Please keep that fast path, as required in #1249.

@sobolevn

Copy link
Copy Markdown
Member Author

@Siyet friendly ping :)
I want to get this PR merged, so I can start working on other fixes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants