◐ Shell
reader mode source ↗
Skip to content

bpo-29438: fixed use-after-free in key sharing dict#17

Merged
methane merged 1 commit into
python:masterfrom
methane:bpo29438/fix-dict-useafterfree
Feb 12, 2017
Merged

bpo-29438: fixed use-after-free in key sharing dict#17
methane merged 1 commit into
python:masterfrom
methane:bpo29438/fix-dict-useafterfree

Conversation

@methane

@methane methane commented Feb 11, 2017

Copy link
Copy Markdown
Member

No description provided.

@methane

methane commented Feb 11, 2017

Copy link
Copy Markdown
Member Author

Patch for Python 3.5 is slightly differ from this PR.
But I'm waiting 3.5 branch has .travis.yml

@methane methane force-pushed the bpo29438/fix-dict-useafterfree branch from c38d9fe to 2dd62f6 Compare February 11, 2017 03:22
@methane methane added the type-bug An unexpected behavior, bug, or error label Feb 11, 2017
@methane methane force-pushed the bpo29438/fix-dict-useafterfree branch from 2dd62f6 to bf8bd7e Compare February 11, 2017 05:48
@codecov

codecov Bot commented Feb 11, 2017

Copy link
Copy Markdown

Codecov Report

Merging #17 into master will increase coverage by <.01%.

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   82.37%   82.37%   +<.01%     
==========================================
  Files        1427     1427              
  Lines      350948   350948              
==========================================
+ Hits       289088   289095       +7     
+ Misses      61860    61853       -7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7ffb99...bf8bd7e. Read the comment docs.

@methane methane merged commit 2294f3a into python:master Feb 12, 2017
@methane methane deleted the bpo29438/fix-dict-useafterfree branch February 12, 2017 04:51
paulmon added a commit to paulmon/cpython that referenced this pull request Jan 10, 2019
jaraco pushed a commit that referenced this pull request Dec 2, 2022
The `html_url` field takes us to the actual comment on GitHub.
`url` field took us to an API JSON response.
nanjekyejoannah added a commit to nanjekyejoannah/cpython that referenced this pull request Dec 13, 2022
17: warn for ssl r=ltratt a=nanjekyejoannah

Warn for `ssl` features.

Co-authored-by: Joannah Nanjekye <jnanjekye@python.org>
pablogsal pushed a commit to DinoV/cpython that referenced this pull request Oct 25, 2025
youknowone referenced this pull request in youknowone/cpython Nov 25, 2025
Also make PyObject an UnsafeCell<ffi::_object> so it can be passed around by & reference
SonicField added a commit to SonicField/cpython that referenced this pull request Apr 19, 2026
Env initialization used hir_chase_assign (only follows Assign instrs)
to compute model registers for support bit assignment. But the pass
processing uses phx_rc_model_reg (follows ALL passthroughs except
GuardIs). When a phi input goes through a non-Assign passthrough
(CheckField, CheckVar, etc.), the two functions return different model
registers. The env assigns a bit for the chase_assign model, but the
pass looks up using the modelReg model — returning -1 (not found).

This -1 was then cast to size_t (becoming SIZE_MAX) and passed to
phx_bv_set_bit, causing out-of-bounds memory access → SIGSEGV.

Fix: use phx_rc_model_reg in env initialization, matching the pass.

Bug python#17: triggered by nbody's phi inputs through non-Assign passthrough
instructions (list subscript operations producing CheckVar intermediaries).
SonicField added a commit to SonicField/cpython that referenced this pull request Apr 22, 2026
Push 47 (88/144 → 89/144 = 61.8%): bundled two-section commit per supervisor
04:48:57Z + theologian 04:48:55Z BUNDLE APPROVED. Section 1 closes a gatekeeper
item python#17 violation from push 46; Section 2 ports emitResume to C.

================================================================
SECTION 1 — FOLLOW-UP FIX (item python#17): JIT_CHECK_C in emitTypeAnnotationGuards
================================================================

Push 46 (5f79084) ported emitTypeAnnotationGuards but the C body used a
silent-skip workaround for the NULL-arg case:

  if (arg == NULL) continue;   // BAD — silent skip, no diagnostic

This violates the no-workarounds policy (feedback_no_workarounds.md): always
root-cause, never bail-out/deopt. Supervisor 04:35:30Z + theologian 04:36:18Z
+ gatekeeper item python#17 (post-push-46 formalization at 04:37:14Z) mandate
ALWAYS-ON form (NOT JIT_DCHECK_C, NOT silent-skip):

  JIT_CHECK_C(arg != NULL, "No register for argument %d", arg_idx);

C++ baseline used JIT_CHECK with the same message format (builder.cpp:1751
pre-deletion). Phoenix C parity restored.

Match-strings (gatekeeper item python#17 grep targets):
  - 'JIT_CHECK_C(arg != NULL'  ← present
  - 'No register for argument' ← present

Header dependency: added '#include "cinderx/Common/jit_log_c.h"' to
builder_emit_c.c for the JIT_CHECK_C macro. No transitive include path
existed.

================================================================
SECTION 2 — EMIT METHOD CONVERSION: emitResume → C (88/144 → 89/144)
================================================================

Bridge: hir_builder_emit_resume_c
Purpose: RESUME opcode handler — inserts an eval-breaker check + periodic
         tasks block when oparg < 2. Oparg 2/3 are no-op resumes
         (coroutine sends/throws don't need periodic-task points).
C++ source: builder.cpp:3175-3186 (replaced with 8-line delegating stub)

PHASE 0 AUDIT:
  Type A (headers — all existing at HEAD):
    - PhxTranslationContext (builder_emit_c.c:22, local typedef)
    - HirFrameStateLayout (phx_frame_state.h)
    - Function/CFG/BasicBlock/FrameState (hir.h, used via void* cast in bridge)

  Type B (symbols — all existing at HEAD):
    - hir_cfg_alloc_block(void *func) — hir_c_api.h:746 ✓ EXISTS
    - hir_c_create_snapshot(void *frame_state) — copies FrameState (hir_c_api.cpp:926-934) ✓ EXISTS
    - phx_tc_emit(tc, instr) — local helper (builder_emit_c.c:39) ✓ EXISTS
    - HIRBuilder::insertRunPeriodicActivites — private member (builder.h:504-508).
      NEW friend bridge added: hir_builder_insert_run_periodic_activities_c
      mirrors hir_builder_get_block_at_off pattern (builder.h:18 fwd-decl +
      builder.h:105 friend ref).

  Phase 0.5 IFDEF AUDIT:
    - insertRunPeriodicActivites contains '#ifdef Py_GIL_DISABLED' block
      (line 5602-5604 emitAtQuiescentState). This stays in C++ helper —
      bridge inherits behavior automatically. NO ifdef in new C body.

  Phase 0 LESSON LEARNED (added during compile-check iteration):
    Initial attempt missed the file-scope FORWARD DECLARATION prerequisite
    for friend bridges. Audit must check BOTH:
      (a) friend-decl site (inside class)
      (b) file-scope forward-decl site (extern "C" block before class)
    Will codify in BRIDGE SPEC TEMPLATE meta-rules.

PRIOR DECISIONS:
  - Bundle approved: supervisor 04:48:57Z + theologian 04:48:55Z (47.1
    surgical 3-line fix; split would require unsafe stash-dance).
  - emitResume choice over emitKwNames: theologian's table LOW + LOW vs
    LOW + LOW-MED — pure velocity contribution per supervisor 04:14:48Z
    REBALANCE directive (no G-item closure value, that's fine).
  - Snapshot-on-succ-block trick (avoids temp PhxTranslationContext + frame
    copy/destroy): theologian 04:45:32Z verified equivalence using
    hir_c_create_snapshot copy semantics at hir_c_api.cpp:926-934
    (snapshot captures frame value at call, subsequent mutations don't
    affect the snapshot — equivalent to C++ succ.frame semantics).

INVARIANTS PRESERVED:
  1. Early-return on oparg >= 2 (no Resume action — matches C++ guard).
  2. Snapshot emitted into succ_block, NOT check_block (matches C++
     succ.emitSnapshot semantic). Trick: capture original tc->block,
     switch tc->block = succ_block, emit snapshot, then restore via
     end-state assignment (final tc->block == succ_block).
  3. insertRunPeriodicActivites runs full helper chain (eval-breaker check
     + body w/ RunPeriodicTasks + branches) — preserved via extern "C"
     delegation through hir_builder_insert_run_periodic_activities_c.
  4. Py_GIL_DISABLED behavior preserved (ifdef stays in C++ helper).
  5. Final tc->block == succ_block (caller continues on new block —
     matches C++ tc.block = succ.block).

DESIGN EQUIVALENCE PROOF (snapshot-on-succ-block):
  C++:  TranslationContext succ(cfg.AllocateBlock(), tc.frame);
        succ.emitSnapshot();
        insertRunPeriodicActivites(cfg, tc.block, succ.block, tc.frame);
        tc.block = succ.block;

  C:    void *check_block = tc->block;
        void *succ_block = hir_cfg_alloc_block(func);
        tc->block = succ_block;
        phx_tc_emit(tc, hir_c_create_snapshot(&tc->frame));
        hir_builder_insert_run_periodic_activities_c(
            builder, func, check_block, succ_block, &tc->frame);

  EQUIVALENT because:
    (a) C++ succ.frame is a COPY of tc.frame at construction; emitSnapshot
        doesn't mutate frame; therefore succ.frame == tc.frame at point of
        insertRunPeriodicActivites call.
    (b) hir_c_create_snapshot makes a COPY of the FrameState
        (hir_c_api.cpp:926-934) — snapshot captures the value at the call,
        not a reference. Subsequent mutations to tc->frame do not affect
        the snapshot.
    (c) Snapshot lands in tc->block at time of phx_tc_emit, which we
        set to succ_block — same destination as C++ succ.emitSnapshot.
    (d) Final tc->block == succ_block (matches C++ tc.block = succ.block).

Falsifier: any function with RESUME opcode (most functions, since RESUME
is emitted at function entry by Python 3.12 compiler). JIT-compile,
verify HIR shows 2-block sequence around RESUME(0|1): check_block ends
with CondBranch on EvalBreaker, succ_block starts with Snapshot. If
single-block or wrong successor, port has stripped invariants.

================================================================
DIFF
================================================================

Python/jit/hir/builder.cpp (-11 / +20):
  - emitResume body (10 lines) → 8-line delegating stub calling
    extern "C" hir_builder_emit_resume_c.
  - +13 lines: extern "C" hir_builder_insert_run_periodic_activities_c
    bridge wrapper (5-line static_cast chain calling private member).

Python/jit/hir/builder.h (+5):
  - +3 lines: file-scope forward declaration of
    hir_builder_insert_run_periodic_activities_c inside existing extern
    "C" block (matches hir_builder_get_block_at_off pattern, line 18).
  - +2 lines: friend declaration inside class HIRBuilder (line 106-107).

Python/jit/hir/builder_emit_c.c (+39 / -5):
  - +1 #include "cinderx/Common/jit_log_c.h" (JIT_CHECK_C macro for §1)
  - §1 fix: silent-skip replaced with JIT_CHECK_C(arg != NULL, ...)
  - +35 lines: hir_builder_emit_resume_c implementation + extern decls.

Net diff stat: 3 files, +64/-12 (net +52 LOC).

================================================================
VERIFICATION (compile-clean pre-commit)
================================================================

cmake --build phoenix_jit: PASS, 0 errors (only 4 pre-existing warnings).
Verified by testkeeper compile-check at 05:02:51Z (after one fix iteration
for missing file-scope forward-decl). Build succeeded with friend
declaration resolving correctly to global namespace function.

Push 47 batch is 1 commit per BUNDLE APPROVED at 04:48:57Z. ABBA_pre
baseline 1.07x captured at HEAD 5f79084 (testkeeper 04:59:13Z) for
cap-20 ABBA_post comparison.

Process discipline applied:
  - explicit `git add Python/jit/hir/builder.cpp Python/jit/hir/builder.h
    Python/jit/hir/builder_emit_c.c` (lesson from 6450421c93 over-broad
    commit; lesson from 04:27:32Z directive-compliance feedback)
  - `git diff --cached --stat` verification: only 3 files staged
  - `git status --short` confirmed file separation pre-staging:
    M  Python/jit/hir/builder.cpp        (mine, staged)
    M  Python/jit/hir/builder.h          (mine, staged)
    M  Python/jit/hir/builder_emit_c.c   (mine, staged)
     M docs/wiring_catches.md            (testkeeper W13a, UNSTAGED)
     M scripts/gate_phoenix.sh           (testkeeper W13a, UNSTAGED)
SonicField added a commit to SonicField/cpython that referenced this pull request Apr 22, 2026
Push 48 Tier 5 emit-method conversion: port emitKwNames from C++ to C per
supervisor 04:14:48Z REBALANCE directive (velocity focus, single emit-method
push). No deletion-gate item closure (post-G2 velocity arc).

================================================================
BRIDGE SPEC TEMPLATE
================================================================

Bridge: hir_builder_emit_kw_names_c
Purpose: KW_NAMES opcode handler — saves the keyword-names tuple from
         co_consts[oparg] into the HIRBuilder kwnames_ slot for the next
         CALL/CALL_KW to consume as part of the call's operands.
C++ source: builder.cpp:3201-3220 (deleted in this commit)

PHASE 0 AUDIT:
  Type A (headers — all existing at HEAD):
    - PyCodeObject (Python.h) — co_consts access
    - PyTupleObject — PyTuple_Size, PyTuple_GET_ITEM
    - PhxTranslationContext (builder_emit_c.c:22, local typedef)

  Type B (symbols — all existing at HEAD):
    - hir_func_alloc_register(func) ✓ EXISTS — equivalent to
      TempAllocator::AllocateNonStack since the latter just calls
      env_->AllocateRegister at builder.cpp:298. NO new bridge needed.
    - hir_c_create_load_const(reg, type) ✓ EXISTS (builder_emit_c.c:723)
    - hir_type_from_object(PyObject*) ✓ EXISTS — equivalent of Type::fromObject
    - phx_tc_emit(tc, instr) ✓ EXISTS (local helper, builder_emit_c.c:39)
    - JIT_CHECK_C ✓ EXISTS (header included since push 47)

  Type B (symbols — NEW BRIDGES, 2):
    - hir_builder_get_kwnames(builder) → void* (Register*)
    - hir_builder_set_kwnames(builder, void *reg)
    Reason for getter+setter pair (vs. single 'do everything' bridge):
    emitCall (next conversion target after kwnames cluster) reads kwnames_
    and clears it post-consume (builder.cpp:3088-3093). Independent
    getter+setter enables that future conversion without re-architecting.

  Phase 0.5 IFDEF AUDIT: N/A (no #ifdef-guarded code in emitKwNames)

  Phase 0 LESSON FROM PUSH 47 APPLIED:
    Both new bridges have:
      (a) friend declarations in HIRBuilder class (builder.h:108-109)
      (b) file-scope forward declarations in extern "C" block (builder.h:28-29)
    Both sites verified BEFORE first compile-check this time.
    No friend-decl resolution error (vs. push 47 first attempt).

PRIOR DECISIONS:
  - emitKwNames choice over CLUSTER (post-G2 velocity arc): supervisor
    05:13:54Z + theologian 04:14:15Z pre-analysis (LOW + LOW-MED). No
    theologian CLUSTER analysis posted before push 48 announcement, so
    default per supervisor.
  - LOW-MED risk caveat is about kwnames touching CALL_KW argument-passing
    semantics. emitKwNames is the PRODUCER side (saves the names tuple);
    the CONSUMER (emitCall) is unchanged in this push. So push 48 risk
    surface is producer-only path, NOT call-graph integration.

INVARIANTS PRESERVED:
  1. Index bounds check: oparg < co_consts length, otherwise abort with
     diagnostic. Matches C++ JIT_CHECK at builder.cpp:3206-3210.
     Implemented as JIT_CHECK_C (ALWAYS-ON, item python#17 form).
  2. Single-slot invariant: kwnames_ MUST be NULL when emitKwNames runs;
     otherwise prior KW_NAMES wasn't consumed by a CALL* opcode. Matches
     C++ JIT_CHECK at builder.cpp:3211-3215. Implemented as JIT_CHECK_C.
  3. Non-stack allocation: kwnames_reg is allocated as a non-stack temp
     (hir_func_alloc_register, NOT phx_ptr_arr_push to stack). Matches
     C++ temps_.AllocateNonStack semantic.
  4. LoadConst with object-type: tuple type captured via
     hir_type_from_object (Type::fromObject equivalent) — gives the
     consumer (emitCall) access to the full PyObject spec for
     resolveKwargs static-resolution path.
  5. NO stack push: kwnames_ is NOT a stack value. The reg lives on the
     HIRBuilder for the next CALL to read. emitLoadConst on TC vs. the
     stack-pushing path (hir_builder_emit_load_const_c) is a critical
     difference — hir_c_create_load_const + phx_tc_emit alone (no
     phx_ptr_arr_push to stack).

Falsifier: Python function with keyword-arg call (e.g. `f(x=1, y=2)`).
JIT-compile, verify HIR shows: KW_NAMES instruction emits LoadConst<TTuple>
to a non-stack reg, followed by CALL with that reg as last operand. If
the kwnames reg appears on the stack OR if CALL receives wrong tuple, port
has stripped invariants.

================================================================
DIFF
================================================================

Python/jit/hir/builder.cpp (-15 / +33):
  - emitKwNames body (~16 lines) → 8-line delegating stub calling
    extern "C" hir_builder_emit_kw_names_c.
  - +12 lines: extern "C" hir_builder_get_kwnames + hir_builder_set_kwnames
    bridge wrappers (3 lines each, static_cast + field access).

Python/jit/hir/builder.h (+4):
  - +2 lines: file-scope forward decls of hir_builder_get_kwnames +
    hir_builder_set_kwnames inside existing extern "C" block (matches
    pattern from push 47).
  - +2 lines: friend decls inside class HIRBuilder.

Python/jit/hir/builder_emit_c.c (+37):
  - +2 extern decls (kwnames getter/setter)
  - hir_builder_emit_kw_names_c implementation (~22 lines)
  - Comment block describing semantics + bridge rationale.

Net diff stat: 3 files, +59/-15 (net +44 LOC).

================================================================
VERIFICATION (compile-clean pre-commit)
================================================================

cmake --build phoenix_jit: PASS, 0 errors (only 4 pre-existing warnings).
Verified by testkeeper compile-check at 05:19:22Z.
Friend declaration + file-scope forward decl both resolved correctly
(Phase 0 audit refinement from push 47 applied successfully).

Push 48 batch is 1 commit per supervisor 04:14:48Z REBALANCE directive
(single emit-method push, velocity focus).

Process discipline applied:
  - explicit `git add Python/jit/hir/builder.cpp Python/jit/hir/builder.h
    Python/jit/hir/builder_emit_c.c` (lesson from 6450421c93 over-broad
    commit; lesson from 04:27:32Z directive-compliance feedback)
  - `git diff --cached --stat` verification: only 3 files staged
  - `git status --short` confirmed file separation pre-staging:
    M  Python/jit/hir/builder.cpp        (mine, staged)
    M  Python/jit/hir/builder.h          (mine, staged)
    M  Python/jit/hir/builder_emit_c.c   (mine, staged)
     M docs/wiring_catches.md            (testkeeper W13a, UNSTAGED)
     M scripts/gate_phoenix.sh           (testkeeper W13a, UNSTAGED)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants