bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs.#75
Conversation
df67bdc to
89c7440
Compare
February 14, 2017 14:32
89c7440 to
76737a6
Compare
February 14, 2017 14:41
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.
|
Sorry, something went wrong.
04111c1 to
7b1dd6e
Compare
February 14, 2017 15:13
|
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? |
Sorry, something went wrong.
|
I updated pull request title and body. |
Sorry, something went wrong.
There was a problem hiding this comment.
For the PyEval_*/PyObject_* functions that are now duplicates, it would be useful to have block comments explaining the equivalence, and the fact the redundant definitions are being kept around for backwards compatibility reasons.
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
3272dba to
e17b495
Compare
March 1, 2017 09:26
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.
e17b495 to
4a3c0d5
Compare
March 1, 2017 09:32
|
I squashed all commit and modified commit message. |
Sorry, something went wrong.
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.
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]