Skip to content

[BUG] set_cover Python: fix UB in vector transforms and swapped GuidedTabuSearch bindings#5121

Open
jg-codes wants to merge 1 commit intogoogle:mainfrom
jg-codes:fix-set-cover-python-bindings
Open

[BUG] set_cover Python: fix UB in vector transforms and swapped GuidedTabuSearch bindings#5121
jg-codes wants to merge 1 commit intogoogle:mainfrom
jg-codes:fix-set-cover-python-bindings

Conversation

@jg-codes
Copy link
Copy Markdown

@jg-codes jg-codes commented Apr 3, 2026

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::transform on empty vectors

VectorIntToVectorSubsetIndex and the all_subsets property both call std::transform writing to subs.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 focus list hits VectorIntToVectorSubsetIndex: next_solution(focus) on all heuristic classes, compute_coverage_in_focus, both clear_random_subsets overloads, and both clear_most_covered_elements overloads. Accessing model.all_subsets from Python triggers the second instance. In practice, the write corrupts heap metadata. I observed crashes on macOS (heap-buffer-overflow) when calling next_solution with a focus list.

Alternatives considered:

  • subs.resize(n) + write to begin() — correct, but default-constructs every element before overwriting. For SubsetIndex the cost is negligible, but reserve + back_inserter avoids even that.
  • Ranged constructor std::vector<SubsetIndex>(ints.begin(), ints.end()) — not viable because the element types differ (BaseIntSubsetIndex), so a transform is needed.

Fix: reserve(n) + std::back_inserter(). This is the same pattern already used in the columns and rows properties in the same file.

Bug 3: swapped getter/setter for GuidedTabuSearch::lagrangian_factor

Lines 558–560 bind the Python name "set_lagrangian_factor" to the C++ method GetLagrangianFactor and vice versa. Calling gts.set_lagrangian_factor(0.5) from Python returns a value instead of setting one; calling gts.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 Exercises
test_all_subsets_property all_subsets property (Bug 2)
test_focus_based_next_solution next_solution(focus) path through VectorIntToVectorSubsetIndex (Bug 1)
test_guided_tabu_search_lagrangian_factor set/get_lagrangian_factor round-trip (Bug 3)

Checklist

  • Read CONTRIBUTING.md and PR template — targeting main as instructed
  • CLA signed
  • Minimal diff — only the bug fixes and their tests
  • No formatting or refactoring changes
  • AI-assisted: disclosed above

@google-cla
Copy link
Copy Markdown

google-cla bot commented Apr 3, 2026

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.

@jg-codes jg-codes force-pushed the fix-set-cover-python-bindings branch from d6afbef to 5a9195b Compare April 3, 2026 18:43
@jg-codes jg-codes changed the title [BUG] Fix undefined behavior and swapped getter/setter in set_cover Python bindings [BUG] set_cover Python: fix UB in vector transforms and swapped GuidedTabuSearch bindings Apr 3, 2026
@jg-codes jg-codes changed the base branch from stable to main April 3, 2026 18:51
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.
@jg-codes jg-codes force-pushed the fix-set-cover-python-bindings branch from 5a9195b to 25129a2 Compare April 3, 2026 20:18
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant