bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs.#75
bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs.#75methane merged 1 commit intopython:masterfrom
Conversation
There was a problem hiding this comment.
did you ment Use PyObject_Call() it seem like this patch remove PyObject_CallObject ... Autocompleter got a bit overzealous ?
There was a problem hiding this comment.
This patch deprecates PyEval_CallObject, not PyObject_CallObject.
There was a problem hiding this comment.
Oh, sorry, I got confused with the unified diff and above comment, the proximity of PyObject_Call() and PyObject_CallObject() made me think you had an Object too much in the comment.
There was a problem hiding this comment.
There are so many similar call APIs.
That's why I think we should deprecate undocumented and less used APIs.
methane
force-pushed
the
deprecate-evalcall
branch
from
df67bdc to
89c7440
Compare
methane
changed the title
methane
force-pushed
the
deprecate-evalcall
branch
from
89c7440 to
76737a6
Compare
Codecov Report
@@ Coverage Diff @@
## master #75 +/- ##
=========================================
Coverage ? 82.38%
=========================================
Files ? 1428
Lines ? 351138
Branches ? 0
=========================================
Hits ? 289281
Misses ? 61857
Partials ? 0Continue to review full report at Codecov.
|
methane
force-pushed
the
deprecate-evalcall
branch
2 times, most recently
from
04111c1 to
7b1dd6e
Compare
|
Sorry but I'm confused by the PR title and the commit messages. They don't describe exactly what I see in the code. Can you please update them? |
methane
changed the title
|
I updated pull request title and body. |
ncoghlan
changed the title
|
@Haypo Would you review this again? |
There was a problem hiding this comment.
I don't understand why you change the code in this commit, and 2 commits later reverts this change. You should practice git rebase -i HEAD~4 to merge the two commits maybe?
There was a problem hiding this comment.
Please update the commit message which doesn't make sense compared to the modified code.
This commit fixes a bug, no? It makes sure that a TypeError is raised if kwargs is not a dictionary, right? Please explain it in the commit message.
There was a problem hiding this comment.
I remove this bugfix for now and update #87 instead.
There was a problem hiding this comment.
commit message: this change also optimizes PyEval_CallFunction() and PyEval_CallMethod() to avoid temporary tuple when possible.
There was a problem hiding this comment.
Main purpose is reducing maintenance cost rather than optimization.
There are very few callers in the world.
I hope compiler can remove duplicated code too, but I'm not sure.
There was a problem hiding this comment.
I don't understand the "are documented" part, maybe remove it?
I suggest to be more explicit:
" PyEval_CallObjectWithKeywords() is kept for backward compatibility: PyObject_Call(), PyObject_CallFunction() and PyObject_CallMethod() are recommended to call a callable object."
|
I'm not confortable yet with GitHub to review commit messages :-/ It's not easy to comment the commit message: I also suggest to add "bpo-29548" to commit messages (at least to one commit message). |
methane
changed the title
I agree. I like Gerrit for this.
Final commit message is written when pushing "Squash and merge" button. I hope it works as poor man's Gerrit for a while, until we have new tool to build Misc/NEWS. |
methane
force-pushed
the
deprecate-evalcall
branch
2 times, most recently
from
3272dba to
e17b495
Compare
PyEval_Call* APIs are not documented and they doesn't respect PY_SSIZE_T_CLEAN. So add comment block to recommend PyObject_Call* APIs where they are declared. This commit also changes PyEval_CallMethod and PyEval_CallFunction implementation same to PyObject_CallMethod and PyObject_CallFunction to reduce future maintenance cost. Optimization to avoid temporary tuple are copied too. PyEval_CallFunction(callable, "i", (int)i) now calls callable(i) instead of raising TypeError. But accepting this edge case is backward compatible.
methane
force-pushed
the
deprecate-evalcall
branch
from
e17b495 to
4a3c0d5
Compare
|
I squashed all commit and modified commit message. |
Co-authored-by: Jules <julia.poo.poo.poo@gmail.com>
Stay at eol when moving up/down
Prevents accidental shallow copy of raw MemoryIndirect* in value_.indirect, which would create double-free risk. The copy constructor deliberately skips value_ (safe); this ensures assignment doesn't silently shallow-copy it. Per gatekeeper recommendation (20:34) and Pythia assessment python#75.
C++ destroys members in reverse declaration order. With cfg before env, env destructs first — freeing Registers while cfg still holds Instructions with FrameState Register* pointers into env memory. Swap: env now declared before cfg, so env outlives cfg during destruction. FrameState's borrowed Register* pointers remain valid throughout cfg teardown. Found by Pythia assessment python#75. Currently safe by coincidence (~FrameState only frees the buffer, never dereferences), but any future dereference during destruction would be use-after-free.
ARM64 pydebug shutdown SEGV root-cause: GlobalCacheKey holds raw
PyDictObject* pointers to globals/builtins. When globals is freed
during Py_FinalizeEx (interpreter_clear → PyDict_Clear), the cache
entry remains in map_ + watch_map_[builtins]. Subsequent
PyDict_EVENT_CLEARED on builtins triggers updateCache, which derefs
cache.key().globals → use-after-free. Pydebug poison fill (0xdd...)
makes the UAF a hard SEGV; x86_64 release reads stale-but-readable
type pointer and silently passes the JIT_CHECK.
Two surgical guards address the FINALIZE-SPECIFIC manifestation:
Part A — Python/jit_support/phoenix_init.cpp
phoenix_dict_watcher early-returns when Py_IsFinalizing(). Cache
state is about to be destroyed in phoenix_free; processing events
on tearing-down dicts is moot. Pattern matches pyjit.cpp:1642
funcDestroyed precedent.
Part B — Python/jit/global_cache.cpp
GlobalCacheManager::clear() short-circuits when Py_IsFinalizing().
Avoids two failure modes:
1. Ci_Watchers_UnwatchDict (PyDict_Unwatch) reads dict internals
on freed memory → UAF under pydebug
2. ci_watcher_state_fini ran first in phoenix_free → watcher_id
is -1 → JIT_CHECK 'Invalid dict watcher ID -1' abort
(observed in earlier gate logs, regression historically)
SCOPE HONESTY: this is FINALIZE-SPECIFIC protection, NOT full
root-cause for the raw-pointer-aliasing class. Mid-execution module
unload (refcount→0 on globals while cache holds raw pointer) remains
unaddressed. Filed as W27 GlobalCacheKey raw-pointer lifecycle
re-architecture (docs/w27-globalcachekey-lifecycle.md, Tier 6+ scope).
The 314d0f2 'weak reference semantics' comment was FALSIFIED — no
mechanism actually provided weak-reference semantics; comments updated
to name the actual mechanisms (Py_IsFinalizing guard, finalize-only
protection).
Diagnostic evidence (per Debug-First protocol):
lldb backtrace on ARM64 pydebug push 59 falsifier:
PyType_HasFeature(type=0xdddddddddddddddd) object.h:966 <-- SEGV
hasOnlyUnicodeKeys() dict.h:52
GlobalCacheManager::updateCache() global_cache.cpp:325
GlobalCacheManager::notifyDictClear() global_cache.cpp:144
phoenix_dict_watcher() phoenix_init.cpp:58
PyDict_Clear → interpreter_clear → Py_FinalizeEx
Test plan post-commit:
testkeeper ARM64 pydebug --clean rebuild + falsifier rerun
(test_phoenix_jit_inline_except_closure under timeout+redirect)
+ push 58 baseline regression check (8b7b935 still passes).
testkeeper x86_64 compile-only PASS verified pre-commit at
binary timestamp 1776874355 (chat L1872 build verification).
Authorization chain: supervisor option (i) at chat L1862, theologian
LEVEL-1 ratification at chat L1861. Pythia python#75 python#1 (trigger isolation)
gates downstream emitCallExceptionHandler queue, NOT this fix.
Per pythia python#76 [chat 16:45Z] + supervisor revision [chat 16:45Z python#3]: filed-not-owned workstreams accrue weed-debt ('a field marked for fallow grows weeds named next year'). W27 was filed at bf8982b without ownership, falsification test, or revisit-trigger. Added §1 frontmatter: - Owner: theologian (design), generalist (implementation) - Schedule: entry-trigger = Tier 5 ZERO C++ complete + no active push-class blocker; exit by start of cinderx Tier 6 cleanup - Falsification test reference: §8 Added §8 falsification test sketch: Lib/test/test_phoenix_w27_module_unload_uaf.py - SKIPPED until W27 work begins - Reproducer: register cache entry → del sys.modules → gc.collect → fire builtins event → expect SEGV pre-W27 / clean post-W27 - Acceptance: reproduces SEGV pre-W27 + passes post-W27, OR W27 scope re-opens if reproducer is harder to trigger than predicted - Discipline: 'falsification-first' — test lands before fix code Added §7 cross-references: - Push 58 ARM64-pydebug coredump finding from testkeeper [chat 16:45Z]: 8b7b935 baseline ALSO produces shutdown coredump on the same falsifier workload (just non-deterministic vs push 59's deterministic SEGV) — independent confirmation that this is pre-existing latent (pythia python#75 b1 outcome) not push-59-port- shape-specific - W27 ownership update authority chain This commit IS supervisor's pythia python#76 python#3 corrective action — turning 'filed' into 'owned, scheduled, testable'. Pattern reusable: same audit will be applied to W23 (verifier hardening), W25 (typed bridges), W26 (build-state hardening) per supervisor [chat 16:45Z python#3] deferral.
5 participants
PyEval_Call* APIs are not documented and they doesn't respect PY_SSIZE_T_CLEAN.
So add comment block to recommend them where PyEval_Call APIs are declared.
This commit also changes PyEval_CallMethod and PyEval_CallFunction implementation
same to PyObject_CallMethod and PyObject_CallFunction.
PyEval_CallFunction(callable, "i", (int)i)now callscallable(i)instead of raising TypeError.I expect this allows compiler to share some code between PyEval_CallFunction and PyObject_CallFunction. But I'm not sure.
[bpo-29548]