[xihaian251]

Problem

ot.bsp.merge_bijections() reads past the end of perms when fewer plans than samples are
supplied, and silently ignores part of its input when more are supplied. Both come from one line:
the number of plans is taken from the wrong axis. Reachable from the documented API:

cost, perm, perms = ot.bsp.compute_bspot_bijection(X, Y, n_plans=1)  # perms: (1, 32)
ot.bsp.merge_bijections(X, Y, perms, p=2)                            # access violation

Reported in #910.

Root cause

perms is a stack of (n_plans, n_samples) permutations — what compute_bspot_bijection returns
and what the docstring promises. merge_bijections_c read the plan count from axis 1, the sample
count, and passed it as nb_plans. MergeBijections() takes n integers per plan, so it consumes
n_samples * n_samples integers from a buffer of n_plans * n_samples:

• n_plans < n_samples reads (n_samples - n_plans) * n_samples integers out of bounds and feeds
  whatever it finds to BijectiveMatching as permutations;
• n_plans > n_samples never reads plans at index >= n_samples, so they are dropped. Measured on
  a 7-plan stack over 4 points: the merge returns cost 3.815 instead of 1.730.

bsp_solve_c passes n_plans explicitly and is not affected.

Fix

• ot/bsp/bsp_wrap.pyx reads the plan count from plans.shape[0].
• ot/bsp/bsp_ot.py rejects, before the int32 conversion, a perms that is not 2-D, whose rows
  are not one entry per point, that is an empty stack, or whose rows are not a permutation of
  0..n_samples-1. The order matters: the cast truncates 3.5 to 3 and keeps the low bits of
  2**32 + 3, either of which would otherwise reach C++ as a valid permutation.
• merge_bijections_c keeps only the two guards that bound the buffer it hands to C++ — the row
  length and the empty stack. Per follow-up review, the permutation-value loop is left to the
  Python wrapper.

No algorithm change: for a well-formed plan stack the merge receives exactly the plans it was
given.

Two different contracts, deliberately

• ot.bsp.merge_bijections (documented API) performs input validation: it rejects a perms
  that is not 2-D, has the wrong row length, is empty, or whose rows are not permutations, with
  messages naming the expected layout. This is the user-facing guarantee.
• merge_bijections_c (internal Cython helper) keeps only two preconditions — row length and a
  non-empty stack. These are not validation; they are what is needed to bound the buffer handed to
  MergeBijections. A well-formed plan stack is a precondition of the helper, and the helper is
  reached directly only by callers that bypass the public function.

This split is what the follow-up review asked for. The consequence is stated under "Known residual
limitation" rather than left implicit.

Known residual limitation, from the ASan work below

With the native permutation loop removed, calling merge_bijections_c directly with a row that is
not a permutation reaches BijectiveMatching::getInverse and performs an out-of-bounds write
(AddressSanitizer: WRITE of size 4, 0 bytes after 32-byte region,
BSP-OT_header_only.h:817, for [0,1,2,3,4,5,6,8] over 8 points; 4 bytes before for
[-1,1,...]). ot.bsp.merge_bijections cannot reach it — the Python check refuses those rows —
but merge_bijections_c is importable (ot.bsp.bsp_wrap). Flagging it so the choice to rely on
the Python wrapper is made knowingly.

Tests

Seven test functions in test/test_bspot.py (the invalid-input matrix is parameterised, so the file
collects 30 tests together with #898's). merge_bijections
had no direct coverage at all before this; the six pre-existing BSP tests all go through
compute_bspot_bijection, which is why the bug survived, and they pass on the unfixed build.
Costs are recomputed in the tests from the returned permutation instead of compared to constants,
so nothing pins a random sequence or a standard library. Reverting only the axis line, with the
guards left in place, makes four tests abort the interpreter and one fail its assertion.

#898 is merged into this branch; both test sets are kept and 30 tests pass.

Memory-safety validation

Ubuntu 24.04 / gcc 13.3, -fsanitize=address -g -O0 -fopenmp, the real bsp_wrap.pyx +
bsp_wrapper.cpp, two variants differing only by the axis line:

input	plans.shape[1]	this branch
(1,32) over 32 points	heap-buffer-overflow, READ 128, 0 bytes after 128-byte region, via MergeBijections → bsp_wrapper.cpp:149	clean, cost=5.501253128
(0,8) over 8 points	heap-buffer-overflow, READ 32, 0 bytes after 1-byte region	ValueError, no report
(3,7) over 8 points	ValueError from the row-length guard	same
(7,4) over 4 points	cost 3.815 (4 of 7 plans used)	cost 1.730 (all 7 used)

Windows/MSVC 14.44 extension: pytest test/test_bspot.py 30 passed;
pytest test/test_solvers.py -k bsp 6 passed; pytest test/test_solvers.py 912 passed,
98 skipped. pre-commit run --all-files clean for ruff (all three hooks) and codespell; yamllint
complains about CRLF in workflow YAML on this Windows checkout, unrelated to these files.

Out of scope

compute_bspot_bijection(..., n_plans=0) still aborts the interpreter: it builds its own plan
stack and never goes through merge_bijections. That is the separate n_plans=0 defect noted
under #898.

Fixes #910

[xihaian251]

Thanks for flagging this as worth its own report -- it is now #911.

The root cause is one axis: merge_bijections_c took the plan count from perms.shape[1], which is
the number of samples, and handed it to MergeBijections() as nb_plans. The C++ loop reads n
integers per plan, so it consumed n_samples * n_samples integers from a buffer of
n_plans * n_samples. n_plans < n_samples read past the end; n_plans > n_samples silently dropped
the plans at index >= n_samples.

The fix reads the count from plans.shape[0] and rejects a perms that does not follow the documented
(n_plans, n_samples) layout -- at the Python boundary for the message, and again in the Cython layer,
because AddressSanitizer shows the overrun is still reachable through merge_bijections_c directly if
only the Python check is added.

Memory safety was verified on the built extension rather than a stand-in: bsp_wrap.pyx was compiled
with -fsanitize=address twice, from master and from this branch, and the same inputs were run
against both. master gives heap-buffer-overflow (READ of size 128, 0 bytes after 128-byte region for the 32-point/1-plan case; READ of size 32, 0 bytes after 84-byte region for a (3, 7)
stack over 8 points), with the stack running through __pyx_pf_8bsp_wrap_2merge_bijections_c into the
std::copy in MergeBijections. This branch returns normally for the first and raises before touching
C++ for the second.

Five tests added, all of which fail without the fix (four abort the interpreter, one fails its
assertion). They assert invariants -- returned permutation validity, reported cost recomputed
independently from that permutation, and the documented "no worse than any merged plan" bound -- rather
than constants, so nothing here pins a particular random sequence or standard library. merge_bijections
had no direct test coverage before, which is how this survived: the six existing BSP tests all go
through compute_bspot_bijection, and they pass on the unfixed build.

Left out on purpose, and stated in the PR rather than hidden: a perms with zero plans still aborts
the interpreter, before and after this change. What changes is only the mechanism -- master reads past
a zero-length allocation, here the reads stay bounded and the failure moves into the merge helper
dereferencing an empty plan vector. That is the separate n_plans=0 bug.

One note for whoever merges: this branch and #898 both append to the end of test/test_bspot.py, so
git merge-tree reports a conflict there in either order (ot/bsp/bsp_ot.py and RELEASES.md
auto-merge). The two changes are otherwise independent.

CI is still running on 615b28c -- 14 checks green so far with build, linux-tf and the CircleCI
docs build pending.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.06%. Comparing base (90484ae) to head (fa8f087).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #911      +/-   ##
==========================================
+ Coverage   97.00%   97.06%   +0.06%     
==========================================
  Files         130      130              
  Lines       26995    27075      +80     
==========================================
+ Hits        26187    26281      +94     
+ Misses        808      794      -14     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[baptiste-genest]

Thanks, it looks good and all tests pass here, including with #898's C++ change.

One thing: invalid perms values still crash or hang. Could you please add a permutation check and reject zero plans too?

if perms.shape[0] == 0:
    raise ValueError("perms must contain at least one plan")
if not np.all(np.sort(perms, axis=1) == np.arange(X_np.shape[0])):
    raise ValueError("each row of perms must be a permutation of 0..n_samples-1")

[baptiste-genest]

I don't feel like the comments are necessary here, this is what the correct implementation should have been from the start, it's my fault ^^

[xihaian251]

Thanks! I've removed the explanatory comments and added validation for empty plan stacks and invalid permutation rows before the native merge call, including at the Cython boundary. Regression tests cover both the public API and direct Cython entry point.

[baptiste-genest]

Thanks, the fix looks right! A few small things before merging: could you remove the permutation loop in merge_bijections_c and its tests, since the Python check should already cover it? and shorten the RELEASES.md entry and the tests a bit please? Once #898 is merged, please also update this branch and keep both sets of tests.

[xihaian251]

Thanks! I've incorporated the requested cleanup in fa8f087:

• Removed the permutation-validation loop from merge_bijections_c, keeping the necessary shape and empty-stack guards.
• Kept permutation validation at the Python API boundary.
• Shortened the tests and RELEASES.md entry.

#898 is now merged, and this branch is up to date with master (0 commits behind). The current checks are green, including CircleCI build_docs.

Could you please take another look?

[baptiste-genest]

Great! Thank you very much!