ITADN

Code review findings (Claude Fable)

#6084Openhenryiii 创建于 2026-06-11
H
henryiiicommented
:robot: _AI text below_ :robot: 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 (25c03cec) 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 — #6088 - [ ] fix: data race on `last_storage_ptr_` in `gil_safe_call_once_and_store` — #6087 - [ ] fix(cmake): stale `PYTHON_MODULE_DEBUG_POSTFIX` cache + `USE_PYTHON_INCLUDE_DIR` variable mismatch — #6086 - [ ] chore: remove Python 2 coding cookie from `eval.h` — #6089 - [ ] chore: minor `setup_helpers.py` cleanups — #6085
1 条评论