[BUG] set_cover Python: fix UB in vector transforms and swapped GuidedTabuSearch bindings#5121
Open
jg-codes wants to merge 1 commit intogoogle:mainfrom
Open
[BUG] set_cover Python: fix UB in vector transforms and swapped GuidedTabuSearch bindings#5121jg-codes wants to merge 1 commit intogoogle:mainfrom
jg-codes wants to merge 1 commit intogoogle:mainfrom
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
Fix three bugs in ortools/set_cover/python/set_cover.cc: 1. VectorIntToVectorSubsetIndex: std::transform wrote to begin() of an empty vector (undefined behavior). Added reserve() + back_inserter(). 2. all_subsets property: same empty-vector UB pattern. Same fix. 3. GuidedTabuSearch: set_lagrangian_factor was bound to GetLagrangianFactor and vice versa. Added regression tests for all three code paths.
5a9195b to
25129a2
Compare
jg-codes
added a commit
to jg-codes/or-tools
that referenced
this pull request
Apr 3, 2026
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.
jg-codes
added a commit
to jg-codes/or-tools
that referenced
this pull request
Apr 3, 2026
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.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes three bugs in the pybind11 wrapper (
ortools/set_cover/python/set_cover.cc), and adds regression tests for each.Disclosure: This PR was drafted with AI assistance (Claude). I reviewed and tested all changes.
Bug 1 & 2: undefined behavior in
std::transformon empty vectorsVectorIntToVectorSubsetIndexand theall_subsetsproperty both callstd::transformwriting tosubs.begin()/subsets.begin()on a default-constructed (empty, capacity 0) vector. The output iterator is invalid from the start — this is undefined behavior per the C++ standard.Impact: Every Python call that passes a
focuslist hitsVectorIntToVectorSubsetIndex:next_solution(focus)on all heuristic classes,compute_coverage_in_focus, bothclear_random_subsetsoverloads, and bothclear_most_covered_elementsoverloads. Accessingmodel.all_subsetsfrom Python triggers the second instance. In practice, the write corrupts heap metadata. I observed crashes on macOS (heap-buffer-overflow) when callingnext_solutionwith a focus list.Alternatives considered:
subs.resize(n)+ write tobegin()— correct, but default-constructs every element before overwriting. ForSubsetIndexthe cost is negligible, butreserve+back_inserteravoids even that.std::vector<SubsetIndex>(ints.begin(), ints.end())— not viable because the element types differ (BaseInt→SubsetIndex), so a transform is needed.Fix:
reserve(n)+std::back_inserter(). This is the same pattern already used in thecolumnsandrowsproperties in the same file.Bug 3: swapped getter/setter for
GuidedTabuSearch::lagrangian_factorLines 558–560 bind the Python name
"set_lagrangian_factor"to the C++ methodGetLagrangianFactorand vice versa. Callinggts.set_lagrangian_factor(0.5)from Python returns a value instead of setting one; callinggts.get_lagrangian_factor()requires an argument and mutates state.The neighboring bindings (
epsilon,penalty_factor,tabu_list_size) are wired correctly — only the lagrangian pair is transposed.Fix: swap the two Python method name strings.
Tests added
test_all_subsets_propertyall_subsetsproperty (Bug 2)test_focus_based_next_solutionnext_solution(focus)path throughVectorIntToVectorSubsetIndex(Bug 1)test_guided_tabu_search_lagrangian_factorset/get_lagrangian_factorround-trip (Bug 3)Checklist
mainas instructed