_ctypes, signal, _multiprocessing: the #1357 review findings - #1371
Conversation
_ctypes
-------
`X_set` allocates the new BSTR first and frees what the slot held only
immediately before the store, so a value it refuses leaves the previous
string readable. Four stores released the slot before the conversion that
can fail — `value_setter` and the struct-field, array-element and
pointer-element stores in `metaclass.rs` — which frees the string and leaves
the freed pointer in the slot. It reads back correctly until the allocator
hands the block to someone else:
b = BSTR("xy"); b.value = 1 # TypeError
[BSTR("ab") for _ in range(64)] # same-size churn reclaims the block
b.value # 'zz' here, 'xy' under CPython
`SysAllocStringLen` answering null now raises `MemoryError` rather than
storing a null pointer.
`_build_callargs` set its `outmask`/`inoutmask` bits with an unbounded
`1 << i`. The masks are the C `int` pair and `_build_result` reads back only
the first 32 bits, so `param_bit` stops at the same width. The same loop held
the values it had collected in a plain `Vec` across `out_parameter`, which
instantiates the argtype and so runs Python; they are pinned as they are
collected and read back out of their root slots.
`PyErr_SetExcFromWindowsErrWithFilenameObjects` reads `GetLastError()` when the
code it is handed is 0, so an explicit zero no longer names `ERROR_SUCCESS`:
SetLastError(5); pythonapi.PyErr_SetFromWindowsErr(0)
# .winerror was 0, is 5
signal
------
The Windows handler flagged the signal and returned, so `set_wakeup_fd` was a
record and no byte was ever written. `signals.c:145-167` writes on both
platforms — the `#ifdef _WIN32` only changes the type of `res` — and
`PYPYSIG_USE_SEND` selects `send` for a descriptor that answered the
`getsockopt` probe. That probe already ran in `set_wakeup_fd`; its verdict now
reaches the handler, which writes the signal-number byte with `send` or the C
runtime's `write`, keeps the EINTR retry and the warn_on_full_buffer drop, and
restores the caller's errno. `rsocket_rffi` gained `WSAEWOULDBLOCK` for the
drop test, and `builtins` a `set_crt_errno` beside `clear_crt_errno`.
Both paths now answer as CPython does:
signal.set_wakeup_fd(sock.fileno()); raise_signal(SIGINT) # b'\x02'
signal.set_wakeup_fd(pipe_w); raise_signal(SIGINT) # b'\x02'
`SIGBREAK` was spelled 21 twice in one file.
_multiprocessing
----------------
`semlock_acquire` saturated a Windows timeout at `u32::MAX`;
`interp_semaphore.py:272-273` refuses one at half of `INFINITE` with
`OverflowError("timeout is too large")`, and rounds with `int(timeout + 0.5)`
rather than upwards. A wait that returned `WAIT_OBJECT_0` then ran
`checksignals_now()`, which can raise after the count has already been taken;
`interp_semaphore.py:311-315` reports the acquisition before anything that can
raise, and a signal pending at that moment is delivered at the next checkpoint.
_thread
-------
`interrupt_main` reported `PyLong_AsLong`'s message for an `i32::try_from`. It
stages through the platform's C `long` first, so MSVC — where a `long` is 32
bits — keeps that one message for everything out of range, and an LP64 build
gets the two `getargs.c` gives its `'i'` conversion.
.gitignore
----------
`@test_*_tmp*` is unanchored: the working directory a stdlib test leaves its
scratch files in is wherever the run started, not the repo root.
Assisted-by: Claude
|
Warning Review limit reached
Next review available in: 6 minutes Limit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91c0c6ddd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn set_wakeup_fd(fd: i32, warn_on_full: bool, use_send: bool) -> i32 { | ||
| WAKEUP_WARN_ON_FULL.store(warn_on_full, Ordering::SeqCst); | ||
| #[cfg(windows)] | ||
| WAKEUP_USE_SEND.store(use_send, Ordering::SeqCst); |
There was a problem hiding this comment.
On Windows, when set_wakeup_fd switches between a pipe and a socket while a signal is delivered concurrently, the handler can observe the new WAKEUP_USE_SEND value with the old WAKEUP_FD: switching pipe→socket then calls send on the old pipe, while socket→pipe calls CRT write on the old socket. Either case can lose the wakeup byte or target an unrelated descriptor with the same numeric value. Store the descriptor and transport selection as one atomic state rather than separate atomics.
AGENTS.md reference: AGENTS.md:L142-L144
Useful? React with 👍 / 👎.
| // `interp_semaphore.py:268-275` — a negative timeout is a poll, | ||
| // and one at half of `INFINITE` (about 25 days) is refused rather | ||
| // than saturated, so no wait silently becomes a different one. |
There was a problem hiding this comment.
The new citation here identifies the upstream behavior only as interp_semaphore.py:268-275, and the commit adds the same line-range pattern for the acquisition branch and several signals.c claims. These references silently rot whenever upstream lines move; name the owning symbols such as semlock_acquire, signal_setflag_handler, and pypysig_set_wakeup_fd and remove the numeric ranges.
AGENTS.md reference: AGENTS.md:L188-L191
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 91c0c6d). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
1 participant
The 15 review findings left on #1357, triaged after it merged. 11 real, 4 refuted.
Fixed
_ctypes— the BSTR store frees before it converts.X_setallocates thenew string first and frees what the slot held only immediately before storing,
so a value it refuses leaves the previous one readable. Four stores had the
opposite order —
value_setterplus the struct-field, array-element andpointer-element stores in
metaclass.rs; the bot reported one. It reads backcorrectly until the allocator hands the block to someone else:
SysAllocStringLenanswering null now raisesMemoryErrorinstead of storinga null pointer.
_ctypes—_build_callargs. Itsoutmask/inoutmaskbits were set withan unbounded
1 << i; the masks are the Cintpair and_build_resultreadsback only the first 32, so
param_bitstops at the same width. The same loopheld its collected values in a plain
Vecacrossout_parameter, whichinstantiates the argtype and so runs Python — they are pinned as collected and
read back out of their root slots.
_ctypes—PyErr_SetFromWindowsErr(0).PyErr_SetExcFromWindowsErrWithFilenameObjectsreadsGetLastError()when thecode is 0, so an explicit zero no longer names
ERROR_SUCCESS:signal— the Windows wakeup fd was never written. The handler flagged thesignal and returned, so
set_wakeup_fdwas a record and nothing reached thedescriptor.
signals.c:145-167writes on both platforms (the#ifdef _WIN32only changes the type of
res) andPYPYSIG_USE_SENDpickssendfor adescriptor that answered the
getsockoptprobe. That probe already ran inset_wakeup_fd; its verdict now reaches the handler, which writes thesignal-number byte with
sendor the C runtime'swrite, keeps the EINTRretry and the
warn_on_full_bufferdrop, and restores the caller's errno.Both paths now answer as CPython does —
b'\x02'for a socketpair and for anos.pipe()write end._multiprocessing.semlock_acquiresaturated a Windows timeout atu32::MAX;interp_semaphore.py:272-273refuses one at half ofINFINITEwith
OverflowError("timeout is too large"), and rounds withint(timeout + 0.5)rather than upwards. A wait returningWAIT_OBJECT_0thenran
checksignals_now(), which can raise after the count has already beentaken —
interp_semaphore.py:311-315reports the acquisition before anythingthat can raise, and a signal pending at that moment is delivered at the next
checkpoint like any other.
_thread.interrupt_mainreportedPyLong_AsLong's message for ani32::try_from. It stages through the platform's Clongfirst, so MSVC —where a
longis 32 bits — keeps that one message for everything out of range,and an LP64 build gets the two
getargs.cgives its'i'conversion.Plus: one
SIGBREAKconstant instead of two, and@test_*_tmp*unanchored in.gitignore(the directory a stdlib test leaves scratch files in is whereverthe run started, not the repo root).
Refuted
c_long_overflowtoc_int_overflow." The oracle disagrees:python3.14 -c "_thread.interrupt_main(2**200)"saysPython int too large to convert to C long. MSVC'slongis 32 bits, soPyLong_AsLongcatcheseverything first. The line did have a real defect, fixed above.
tznamearm needs the POSIX" "fallback."interp_time.py:517-527's_WINbranch has no fallback; it is POSIX-only,for an empty
tm_zone.host_env." Real premise, wrong scope.The Windows
--no-default-featuresbuild fails with 17 errors, 15 of thempre-existing in mmap/sys/importing/posix (
git blameagainst the merge base).Gating two turns compile errors into silent behaviour changes without making
anything build, and nothing uses that config — pyrex reaches
pyre-interpreterthroughpyre-jitwith default features, sohost_envisalways on.
S_OKbefore reading COM error info."GetComErrorgatesInterfaceSupportsErrorInfoonSUCCEEDED(hr), soS_FALSEpasses, andreserves the strict
S_OKtest forGetErrorInfo. That is exactly the splitalready implemented (
supports < 0vsGetErrorInfo(...) != 0).Not done
The rooting fix closes the window inside
build_callargs, which is what wasreported. A wider one remains:
cfuncptr_callholdscallargs.argsacrossmarshal_typed_arg(which runsfrom_param) with nothing rooted, and_build_resultreads those raw refs again after the call. Closing it meansrestructuring the FFI call path, so it is left out of a review-fix change.
Verification
cargo test --all --no-default-features --features dynasmexit 0.extra_tests/parity_tests441/441 across cpython, dynasm and cranelift.pyre/check.py— dynasm 442/442, cranelift 442/442.One red, on a leg this platform's CI does not run:
wasm fib_recursive ratio 5.9x > gate 4x, reproducible at 5.5x / 5.9x / 6.0x over three runs. Windowskeeps no wasm32 target (
pyre-ci.yml:625-632), the fixture's recorded wasmjit-stats match in every run so the compiled trace is unchanged, and the 4x
ceiling with its
fib_recursive 3.6xdatum was set in #1272 on a Linux-onlywasm leg. Not measured against a pre-change control build.