boxstacks: fix out-of-range fixed_items_solutions index in sequential_onedimensional_rectangle - #541
boxstacks: fix out-of-range fixed_items_solutions index in sequential_onedimensional_rectangle#541HansBug wants to merge 2 commits into
Conversation
…_onedimensional_rectangle When the middle axle weight constraint fails, the repair loop rescans fixed_items_solutions for the position to fix next. On the last entry, 'pos + 1 < fixed_items_solutions.size()' is false, so the break was skipped and the counter was incremented once more, leaving fixed_items_solutions_pos equal to fixed_items_solutions.size() whenever no earlier break happened. The clamp that follows only raises the index to the lower bound and the loop's end condition only breaks when the lower bound passes the upper bound, so the next iteration indexed one past the end and copy-constructed a Solution from that memory, which threw std::bad_array_new_length (std::bad_alloc for some objectives). Break on the last entry instead of running past it, so the index stays within the vector.
Upstream PR fontanf#541, issue fontanf#537.
|
Hi, Thank you for the contribution. Do you have a failing test case for this? |
Two instances that enter the repair loop of sequential_onedimensional_rectangle, in a new parametrized suite over instance files, following the other parametrized tests of the project. Both throw std::bad_array_new_length without the fix. The bin packing case covers the objective the crash was reported with; the repair returns an empty solution for it, so the case only pins down that optimize returns at all. The knapsack case is the one that also pins down a non-empty result. The expected number of items is a minimum rather than an exact count, so the test does not have to be updated if the repair later packs more items. boxstacks has no SolutionBuilder::read, so the solutions are checked this way rather than against reference certificates.
|
Yes, added as a second commit, in the same shape as the tests on #540.
Two cases rather than one, because they pin down different things. The bin packing case is the objective the crash was reported with, but the repair returns an empty solution for it even after the fix, so on its own it would only prove that The expected number of items is a minimum rather than an exact count, so the test does not need updating if the repair later packs more items than it does today. Same caveat as on #540: Full build in the default configuration, |
2 participants
Fixes #537
Summary
boxstacksbin is a semi-trailer truck and the first packing violates the middle axle weight limit, the axle weight repair loop insequential_onedimensional_rectangleleftfixed_items_solutions_posequal tofixed_items_solutions.size(), and the next iteration read one element past the end of the vector. Copy-constructing aSolutionfrom that memory threwstd::bad_array_new_length, orstd::bad_allocdepending on the objective.breakon the last entry, because thepos + 1 < fixed_items_solutions.size()guard is false there, so the counter was incremented one time too many.Changes
src/boxstacks/sequential_onedimensional_rectangle.cpp— one condition, so the scan breaks on the last entry instead of running past it:Nothing else was needed. The clamp below it only raises the index to the lower bound, and the loop's own end condition covers the case where the last entry does fail the length test: the lower bound then becomes
size()and the loop breaks before anything is indexed.Test plan
-DCMAKE_BUILD_TYPE=Release, CLP and HiGHS both on,liblapack-devandlibbz2-devinstalled, as in.github/workflows/build.yml).ctest --parallel 8inbuild/test: 596 tests pass, unchanged frommaster.sequential_onedimensional_rectangle(std::bad_array_new_length) #537, built from this branch: 3/3 runs exit 0 and write a certificate. The same build with this commit reverted: 3/3 runs fail withError: std::bad_array_new_length.--objective knapsackand--objective feasibility, which fail withstd::bad_array_new_lengthandstd::bad_alloconmaster: both exit 0 with this change.fixed_items_solutions_pos 2 with fixed_items_solutions.size() 2on the second iteration.I did not add a regression test. Reaching this code needs a full
optimizerun on a semi-trailer instance, and neithertest/boxstacksnordata/boxstackscurrently has a data-driven optimize suite to hang it on. If you would like one, I am glad to add it in the shape you prefer, either adata/boxstacks/tests/...set likerectangleguillotinehas, or a unit test on the loop extracted into its own function.As noted in #537, this stops the crash but the instance still returns an empty solution, which looks like a separate limitation of the repair strategy. This PR does not touch that.
Environment: Ubuntu 24.04.3 LTS, x86-64, glibc 2.39, GCC 13.3.0, CMake 4.4.3.