[BUG] set_cover Python: fix UB in the all_subsets property - #5121
[BUG] set_cover Python: fix UB in the all_subsets property#5121jg-codes wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
d6afbef to
5a9195b
Compare
5a9195b to
25129a2
Compare
Exposes the existing C++ SetCoverLagrangian class to Python via pybind11. Resolves the TODO(user) at the end of set_cover.cc. Bindings cover all public methods: multiplier initialization, reduced costs, subgradient, Lagrangian value, multiplier updates, gap computation, three-phase algorithm, lower bound computation, and their parallel variants. Also includes the bug fixes from google#5121 (UB in vector transforms, swapped GuidedTabuSearch getter/setter) since this patch builds on the corrected file. Added BUILD dep on :set_cover_lagrangian and 6 regression tests.
Exposes the existing C++ SetCoverLagrangian class to Python via pybind11. Resolves the TODO(user) at the end of set_cover.cc. Bindings cover all public methods: multiplier initialization, reduced costs, subgradient, Lagrangian value, multiplier updates, gap computation, three-phase algorithm, lower bound computation, and their parallel variants. Also includes the bug fixes from google#5121 (UB in vector transforms, swapped GuidedTabuSearch getter/setter) since this patch builds on the corrected file. Added BUILD dep on :set_cover_lagrangian and 6 regression tests.
I've signed the CLA. Anything else needed from your side? |
Reading model.all_subsets from Python terminates the interpreter. Two separate defects in the same statement: 1. SetCoverModel::all_subsets() returns by value (set_cover_model.h:211), and the property called it twice, so begin() and end() came from two distinct temporaries. That iterator pair does not delimit a range, so the transform walks off the end of the first temporary's buffer. This is the dominant defect and it is the reason the property has never worked. 2. The output iterator was subsets.begin() on a default-constructed, zero-capacity vector, which is a write through an invalid iterator. Both are fixed by binding the returned vector to a local and appending through back_inserter(). Binding the local also removes a third full copy of the vector that the reserve() call would otherwise make. Note the neighbouring columns and rows properties are not affected: columns() and rows() return const references (set_cover_model.h:189, :192), so calling them twice is harmless. They pre-size and write through begin(), which is correct for a reference-returning accessor. VectorIntToVectorSubsetIndex had defect 2 as well and is fixed here for consistency, but it is not reachable from Python: all of its callers take absl::Span, and this module registers no absl::Span type caster. That is reported separately. Adds a regression test for the property. The focus-based paths cannot be regression-tested from Python until the Span casters are addressed. The swapped GuidedTabuSearch lagrangian getter/setter reported in the first version of this PR was fixed upstream in a1e13b3, so that hunk is dropped.
25129a2 to
dab4410
Compare
|
@Mizux, rebased onto current
|
|
Filed the Span/focus-API defect as its own issue: #5278. |
What
Fixes undefined behaviour in the
set_coverPython wrapper (ortools/set_cover/python/set_cover.cc) and adds a regression test.Disclosure: drafted with AI assistance (Claude). See the verification section for exactly what was built and run.
The bug
SetCoverModel.all_subsetsterminates the interpreter when read from Python. The property isset_cover.cc:201-213, and it has two independent defects in one statement:begin()andend()come from different objects.SetCoverModel::all_subsets()returns by value (set_cover_model.h:211):The property calls it twice, so
model.all_subsets().begin()andmodel.all_subsets().end()are iterators into two distinct temporaries. That pair does not delimit a valid range, and the transform runs off the end of the first temporary's buffer. This is the dominant defect, and it is why the property has never worked.The output iterator is also invalid.
subsets.begin()on a default-constructed, zero-capacity vector: a write through an iterator that was never valid.The neighbouring
columnsandrowsproperties are fine:columns()androws()return const references (:189,:192), so calling them twice is harmless. Their pre-size-and-write-through-begin()pattern is correct for a reference-returning accessor.all_subsets()is the only by-value accessor in the file, and it is the only one that crashes.Reproduction
Segmentation fault, exit 139, on the released
ortools9.15.6755 wheel (macOS arm64, CPython 3.13.11). Thestd::transformstatement at the v9.15 tag is identical to the one onmain.The fix
Bind the returned vector to a local, then append through
back_inserter. Binding the local also removes a third full copy thatreserve(model.all_subsets().size())would otherwise make.VectorIntToVectorSubsetIndexhas the second defect too, and is fixed here for consistency, though it is not currently reachable from Python: all of its callers takeabsl::Span, and this module registers noabsl::Spantype caster. That is a separate, unrelated defect, reported independently rather than folded into this PR.Verification
Built and tested on Linux (Bazel 8.7.0, x86_64) against
mainatd9c0910:clang-formatclean against the repo.clang-format.Checklist
mainall_subsetsfix and its test