Code review findings (Claude) #6084

Tried to use Claude Fable, but I think it might have been just after the cutoff and it used Opus instead - not clear from my history.

🤖 AI text below 🤖

A code review pass (agent-assisted, findings manually verified) turned up one confirmed memory-safety bug, one concurrency issue, and a few smaller items. Filing them together here; draft PRs will reference this issue.

1. bind_vector __delitem__ with negative-step slices: wrong elements deleted / out-of-bounds erase

include/pybind11/stl_bind.h (__delitem__(slice)) uses start += step - 1, which is only correct for positive steps (where erasing shifts subsequent elements down by one). For negative steps, elements below start don't shift, so the -1 correction deletes the wrong elements and eventually erases out of bounds.

Verified with a minimal bind_vector<std::vector<int>> module:

• del v[::-2] on [0, 1, 2, 3] produced [1, 2] instead of [0, 2] — silent data corruption
• del v[::-1] ends up calling v.erase(v.begin() - 1) — UB; observed both a SIGBUS crash and a "correct-looking" result on different runs

There is currently no test coverage for vector slice deletion (only test_map_delitem exists).

Adjacent: the same function contains if (step == 1 && false) — a debugging artifact from the original 2016 stl_bind redesign (25c03ce) that has disabled the O(n) contiguous-erase fast path ever since, making del v[1:1000] perform 999 separate erase calls.

2. gil_safe_call_once_and_store (subinterpreter branch): data race on last_storage_ptr_

include/pybind11/gil_safe_call_once.h (get_stored()):

1. Data race under free-threading. last_storage_ptr_ is a plain T*, written in call_once_and_store_result() / get_stored() and read in get_stored() with no synchronization (under Py_GIL_DISABLED, gil_scoped_acquire provides no mutual exclusion). The pointer is also read before the is_last_storage_valid() check, so even an atomic flag doesn't order it — a thread could pass validation having read a stale/null pointer. Fix: make last_storage_ptr_ std::atomic<T*> and load it after the validity check.
2. Embedded finalize/re-init staleness (medium confidence). is_initialized_by_at_least_one_interpreter_ is never reset, and has_seen_non_main_interpreter() stays false if only the main interpreter is used. After finalize_interpreter() + initialize_interpreter(), the fast path would return the cached pointer into the destroyed interpreter's state-dict capsule. Not yet reproduced; needs discussion on whether/where to hook a reset.

3. CMake: stale cache / variable mismatch

• tools/pybind11NewTools.cmake — when the Python executable changes between runs, PYTHON_IS_DEBUG and PYTHON_MODULE_EXTENSION are unset from cache but PYTHON_MODULE_DEBUG_POSTFIX is not, so a stale postfix from the previous interpreter survives (matters for Windows debug builds switching interpreters in one build dir).
• CMakeLists.txt (~line 307) — the branch checks DEFINED PYTHON_INCLUDE_DIR (singular) but uses ${PYTHON_INCLUDE_DIRS} (plural). Latent today because FindPythonLibsNew.cmake sets both, but breaks if the user pre-seeds only the singular form. Also, USE_PYTHON_INCLUDE_DIR silently no-ops on the FindPython3 path since Python3_INCLUDE_DIRS matches neither branch.

4. eval.h: Python 2 leftover coding cookie

Every py::eval/py::exec prepends "# -*- coding: utf-8 -*-\n" to the source. Python 3's PyRun_String already assumes UTF-8, so this is removable — dropping it removes a string copy and stops shifting line numbers by one in tracebacks/SyntaxErrors from evaluated code.

5. Small cleanups in setup_helpers.py

• cpp_flag_cache = None is dead code (caching is the @lru_cache on auto_cpp_level); the comment above it is stale too.
• def no_recompile(obg: ...) — obg is a typo for obj.

Planned draft PRs (each referencing this issue):

• fix(stl_bind): negative-step slice deletion + re-enable contiguous fast path, with tests — fix(stl_bind): correct __delitem__ for negative-step slices and re-enable contiguous erase fast path #6088
• fix: data race on last_storage_ptr_ in gil_safe_call_once_and_store — fix: data race on last_storage_ptr_ cache in gil_safe_call_once_and_store #6087
• fix(cmake): stale PYTHON_MODULE_DEBUG_POSTFIX cache + USE_PYTHON_INCLUDE_DIR variable mismatch — fix(cmake): unset stale PYTHON_MODULE_DEBUG_POSTFIX and correct USE_PYTHON_INCLUDE_DIR variable #6086
• chore: remove Python 2 coding cookie from eval.h — fix: drop Python 2 coding cookie prepended to evaluated source #6089
• chore: minor setup_helpers.py cleanups — chore(setup_helpers): remove dead cpp_flag_cache and fix parameter typo #6085