Skip to content

Close four more memory-safety holes on the untrusted-input / public-API paths - #161

Merged
BDonnot merged 5 commits into
n1_full_computefrom
claude/security-audit-review-jghnx0
Aug 3, 2026
Merged

Close four more memory-safety holes on the untrusted-input / public-API paths#161
BDonnot merged 5 commits into
n1_full_computefrom
claude/security-audit-review-jghnx0

Conversation

@BDonnot

@BDonnot BDonnot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Follow-up security audit of the n1_full_compute branch, in the spirit of #155 / #156 / #157, over the channels docs/security.rst covers (python bindings, pickle, the binary format) plus the new ContingencyAnalysis (n-1) code path that the earlier audits predate.

Four issues, each reproduced as a segfault / heap corruption before the fix and as a clean exception (or safe behaviour) after. Release wheels are -O3 -DNDEBUG, so neither Eigen's nor the standard library's own bounds checks catch any of these.

Findings

# Issue Trigger Before the fix
1 ContingencyAnalysis stale results → out-of-bounds write ca.add_n1(x) after compute(), then compute_flows() free(): invalid pointer, heap corruption
2 OneSideContainer::update_topo → out-of-bounds read set_load_pos_topo_vect([2**28, …]) then update_topo() SIGSEGV
3 check_grid accepted vmin/vmax with mismatched presence → OOB read pickle / binary with bus_vmin_kv set, bus_vmax_kv empty SIGSEGV (state passed check_grid())
4 LSGrid sub-object getters → use-after-free make_grid().get_lines() then drop the grid disclosed freed heap (a 53M-element "bus id" array), SIGSEGV

Details

  1. ContingencyAnalysis stale results → OOB write. compute() sizes the result matrices (_voltages and the flow matrices derived from it) with one row per registered contingency. Adding / removing contingencies afterwards, then calling compute_flows() / compute_power_flows() without recomputing, left clean_flows() (and the per-contingency violation loop) walking the new, longer _li_defaults while indexing the old, shorter matrices. compute_flows() / compute_power_flows() now refuse the stale state with a clear message.

  2. OneSideContainer::update_topo → OOB read. has_changed / new_values are indexed by the stored pos_topo_vect with an unchecked Eigen operator(). check_grid() proves those positions form a permutation of [0, dim_topo), but the set_*_pos_topo_vect() setters only length-check their argument, not its values — so a position written straight through a setter (bypassing check_grid) read past the caller arrays. Validate el_pos against has_changed.rows() before indexing, same idiom as the entry-path checks in Fix out-of-bounds memory access from Python bindings + RAII for linear-solver memory #143.

  3. vmin/vmax mismatched presence → OOB read. bus_vmin_kv_ / bus_vmax_kv_ are each individually optional (empty-or-complete), but ContingencyAnalysis::check_bus_voltage_violations consumes them together: it loops to vmin.size() and then reads vmax(grid_id). A state with one set and the other empty passed every per-field check yet over-read the empty one (reachable from a pickle / a binary file). check_grid now requires them to have the same presence.

  4. LSGrid getters → use-after-free. get_lines / get_generators / get_substations / … and the Eigen-view getters get_bus_vn_kv / get_V_solver / … were bound with return_value_policy::reference, which does not keep the parent grid alive. Dropping the grid while holding the returned container / numpy view left it dangling. Switch the getters that return a reference / view into grid-owned memory to reference_internal so their lifetime is tied to the grid (same fix for AlgorithmSelector::get_fdpf_{xb,bx}_lu).

Docs

Adds a subsection to docs/security.rst stating the invariant these fixes uphold explicitly: an LSGrid must raise, never corrupt memory — whatever it is handed (malformed pickle/binary, a grid from a source file, or an ordinary API call in the wrong order or with an out-of-range id), the worst it may do is raise a clean Python exception. A genuine crash instead of an exception is a bug to report.

Tests

  • 13 new python cases in test_state_poisoning.py (four new test classes, one per finding).
  • One new test_check_grid.cpp case for the vmin/vmax presence check.
  • Full C++ suite (128 tests) green; the python poisoning suite passes.

Not covered / blind spots

This audit concentrated on the untrusted-input and public-API boundary. It did not deeply review the numeric solver internals (powerflow_algorithm/), the external sparse-solver wrappers (linear_solvers/), or each concrete element container's fillYbus / compute_results indexing. Running the repo's ASan/UBSan CI over the full suite is the recommended next step to reach those.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Vxxj4GukTFz1TKh52GZxLP

Generated by Claude Code

claude added 2 commits August 3, 2026 05:51
@claude
Close four more memory-safety holes found auditing the n1_full_comput…
9aa07ea 
…e branch

Follow-up security audit in the spirit of PRs #155/#156/#157, over the
channels docs/security.rst covers (python bindings, pickle, the binary
format) plus the new ContingencyAnalysis (n-1) code path. Four issues, each
reproduced as a segfault / heap corruption before the fix and as a clean
exception (or safe behaviour) after. Release wheels are -O3 -DNDEBUG, so
neither Eigen's nor the standard library's own bounds checks catch any of
these.

* ContingencyAnalysis stale results -> out-of-bounds write. compute() sizes
  the result matrices (_voltages and the flow matrices derived from it) with
  one row per registered contingency. Adding / removing contingencies
  afterwards, then calling compute_flows() / compute_power_flows() without
  recomputing, left clean_flows() (and the per-contingency violation loop)
  walking the new, longer _li_defaults while indexing the old, shorter
  matrices -- a plain `ca.add_n1(...)` after compute() corrupted the heap
  ("free(): invalid pointer"). compute_flows() / compute_power_flows() now
  refuse the stale state with a clear message.

* OneSideContainer::update_topo -> out-of-bounds read. has_changed /
  new_values are indexed by the stored pos_topo_vect with an unchecked Eigen
  operator(). check_grid() proves those positions form a permutation of
  [0, dim_topo), but the set_*_pos_topo_vect() setters only length-check
  their argument, not its values -- so a position written straight through a
  setter (bypassing check_grid) read past the caller arrays. Validate el_pos
  against has_changed.rows() before indexing, same idiom as the entry-path
  checks added in #143.

* check_grid accepted vmin/vmax with mismatched presence -> out-of-bounds
  read. bus_vmin_kv_ / bus_vmax_kv_ are each individually optional
  (empty-or-complete), but ContingencyAnalysis::check_bus_voltage_violations
  consumes them together: it loops to vmin.size() and then reads
  vmax(grid_id). A state with one set and the other empty passed every
  per-field check yet over-read the empty one (reachable from a pickle / a
  binary file). check_grid now requires them to have the same presence.

* LSGrid sub-object getters -> use-after-free. get_lines / get_generators /
  get_substations / ... and the Eigen-view getters get_bus_vn_kv /
  get_V_solver / ... were bound with return_value_policy::reference, which
  does NOT keep the parent grid alive. Dropping the grid while holding the
  returned container / numpy view left it dangling: reading through it
  disclosed freed heap (a 53M-element "bus id" array) and segfaulted.
  Switch the getters that return a reference / view into grid-owned memory
  to reference_internal so their lifetime is tied to the grid. Same fix for
  AlgorithmSelector::get_fdpf_{xb,bx}_lu.

Tests: 13 new python cases in test_state_poisoning.py and a new
test_check_grid.cpp case for the vmin/vmax presence check. Full C++ suite
(128 tests) and the python poisoning suite pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxxj4GukTFz1TKh52GZxLP
Signed-off-by: Claude <noreply@anthropic.com>
@claude
docs(security): state the "raise, never corrupt memory" invariant exp…
7d5016a 
…licitly

Add a subsection making the core design rule the previous audits enforce
explicit for readers: whatever an LSGrid is handed -- a malformed pickle /
binary file, a grid from a source file, or an ordinary API call in the wrong
order or with an out-of-range id -- the worst it may do is raise a clean
Python exception, never read/write out of bounds, use freed memory, or leave
the grid in a state that makes the next call do so. Notes that this is about
safety (not answer-correctness) and that a genuine crash instead of an
exception is a bug to report.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxxj4GukTFz1TKh52GZxLP
Signed-off-by: Claude <noreply@anthropic.com>
@BDonnot
BDonnot force-pushed the claude/security-audit-review-jghnx0 branch from 0c9393e to 7d5016a Compare August 3, 2026 05:56
BDonnot
BDonnot commented Aug 3, 2026
View reviewed changes

@BDonnot BDonnot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes needed

Comment thread src/core/batch_algorithm/ContingencyAnalysis.hpp
Comment thread src/core/element_container/OneSideContainer.hpp
Comment thread src/core/element_container/OneSideContainer.hpp
claude added 3 commits August 3, 2026 06:52
@claude
Address review: eager result invalidation, and validate pos/subid values
a5b8a5b 
Three follow-ups from review of #161, each a real (proved before/after)
"silently wrong data" bug rather than a crash, closed with the same
validate-eagerly philosophy as the rest of this audit.

* ContingencyAnalysis: a size-only staleness check (added in the previous
  commit) cannot tell remove_n1(x) + add_n1(y), y != x, apart from a clean
  state -- both leave _li_defaults at the SAME size it was at compute()
  time, just with different membership. compute_flows() would then
  silently return flows that correspond to the wrong contingencies instead
  of raising (reproduced: same array, different values, no exception).
  Every mutating method (add_all_n1 / add_n1 / add_multiple_n1 / add_nk /
  remove_n1 / remove_multiple_n1 / remove_nk) now eagerly clears any
  previously computed results via clear_results_only(), closing the gap
  for any sequence of calls rather than only the ones a size comparison
  happens to catch. Validation runs to completion before any mutation (or
  clearing) happens, so a rejected id leaves both the contingency set and
  any existing results untouched -- the batch methods now validate every
  id first, then mutate, rather than interleaving the two.

* OneSideContainer::set_pos_topo_vect / set_subid: only length-checked
  their argument, deferring all value validation to update_topo() (fixed
  in the previous commit) or to an explicit check_grid() call that the
  caller might never make. Reject a negative value immediately in the
  setter itself -- the only bound it can check locally, since the upper
  bound (dim_topo for positions, n_sub for substation ids) depends on
  context the setter does not have (other containers' element counts /
  SubstationContainer, respectively).

* OneSideContainer::update_topo: a substation id feeds
  SubstationContainer::local_to_gridmodel's unchecked arithmetic
  (sub_id + (busbar - 1) * n_sub) to resolve the target bus. That
  function's own bound check is on the OUTPUT (the resolved bus id, which
  is what actually gets used downstream), not on sub_id -- so an
  out-of-range sub_id can land BY COINCIDENCE on another substation's
  legitimate bus id and silently reconnect the element to the WRONG bus
  rather than raising (reproduced: load reassigned to an unrelated
  substation's bus, no exception). Validate sub_id against
  substations.nb_sub() where that context is available, mirroring the
  el_pos check added for pos_topo_vect in the previous commit; also guard
  the case where subid_ was never set at all (empty), which indexed an
  empty vector.

Tests: 7 new python cases (test_state_poisoning.py) and 4 new C++ cases
(test_check_grid.cpp), all reproduced as failing before the fix and
passing after. Full suites green: 132 C++ tests, 46 python tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxxj4GukTFz1TKh52GZxLP
Signed-off-by: Claude <noreply@anthropic.com>
@claude
Fix CI: pre-existing negative pos_topo_vect/subid tests expected the …
649576e 
…old,

setter-does-not-validate contract

The Sanitizers workflow's "ASan + UBSan tests" job failed on a5b8a5b: not a
memory-safety issue (no sanitizer report -- both new C++ checks are unit-tested
directly in test_check_grid.cpp and pass clean under ASan/UBSan), but a
pre-existing Python test file (lightsim2grid/tests/test_check_grid.py, distinct
from this audit's own test_state_poisoning.py) that predates this PR and
assumed set_load_pos_topo_vect() / set_load_to_subid() / set_gen_to_subid()
would silently accept a negative value and defer the rejection to a later
check_grid() call.

That assumption no longer holds: the previous commit made those setters reject
a negative value immediately (the one bound they can check locally). This is a
strictly earlier, more specific rejection of the exact same invalid input --
not a change in what is ultimately accepted -- so check_grid() never gets a
chance to see the poisoned state in these two tests. Move the assertRaises to
wrap the setter call instead of the check_grid() call; the "out of range but
non-negative" siblings (test_out_of_range_pos_topo_vect_is_rejected,
test_out_of_range_subid_is_rejected, test_subid_just_out_of_range_is_rejected)
are untouched, since that bound still needs check_grid()'s cross-container
context and continues to pass as before.

Confirmed no other caller (production LightSimBackend / pypowsybl loader, or
any other test file) relies on the old silent-acceptance behavior: grep across
lightsim2grid/ for every set_*_pos_topo_vect / set_*_to_subid call site shows
production code only ever passes grid2op/pypowsybl-derived, always-valid,
non-negative data.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxxj4GukTFz1TKh52GZxLP
Signed-off-by: Claude <noreply@anthropic.com>
@claude
Guard update_topo's subid_ against being stale relative to the curren…
a07b07b 
…t nb()

Follow-up to the sub_id fix in a5b8a5b, prompted by a review question: that
fix checked subid_.size() == 0 ("never set") but not subid_.size() != nb()
("stale"), even though the exact same class of check already exists for
pos_topo_vect_ (_check_pos_topo_vect_filled()'s second condition).

subid_ is only ever (re)assigned by set_subid() -- checked against nb() at
the time of that call -- or by set_osc_state() (pickle / binary load). init()
does not touch it. So a container re-initialized with MORE elements after
set_subid() was called for the smaller count leaves subid_ shorter than the
container's current nb(): indexing subid_(el_id) for an el_id past its old
length reads out of bounds. Reproduced: init_loads(1 load) + set_load_to_subid
+ init_loads(3 loads) [reusing the same LSGrid, subid_ left at its old size 1]
+ set_load_pos_topo_vect(sized to 3) + update_topo() reconnecting the 3rd load
-- no exception, over-read reachable.

Add the missing subid_.size() != nb() check, mirrored from
_check_pos_topo_vect_filled(). One new C++ test
("update_topo rejects a subid left stale after the container grew").

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxxj4GukTFz1TKh52GZxLP
Signed-off-by: Claude <noreply@anthropic.com>
BDonnot
BDonnot commented Aug 3, 2026
View reviewed changes

@BDonnot BDonnot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@BDonnot
BDonnot merged commit 1b065fd into n1_full_compute Aug 3, 2026
16 checks passed
BDonnot added a commit that referenced this pull request Aug 3, 2026
@claude @BDonnot
Address review: eager result invalidation, and validate pos/subid values
81261ea 
Three follow-ups from review of #161, each a real (proved before/after)
"silently wrong data" bug rather than a crash, closed with the same
validate-eagerly philosophy as the rest of this audit.

* ContingencyAnalysis: a size-only staleness check (added in the previous
  commit) cannot tell remove_n1(x) + add_n1(y), y != x, apart from a clean
  state -- both leave _li_defaults at the SAME size it was at compute()
  time, just with different membership. compute_flows() would then
  silently return flows that correspond to the wrong contingencies instead
  of raising (reproduced: same array, different values, no exception).
  Every mutating method (add_all_n1 / add_n1 / add_multiple_n1 / add_nk /
  remove_n1 / remove_multiple_n1 / remove_nk) now eagerly clears any
  previously computed results via clear_results_only(), closing the gap
  for any sequence of calls rather than only the ones a size comparison
  happens to catch. Validation runs to completion before any mutation (or
  clearing) happens, so a rejected id leaves both the contingency set and
  any existing results untouched -- the batch methods now validate every
  id first, then mutate, rather than interleaving the two.

* OneSideContainer::set_pos_topo_vect / set_subid: only length-checked
  their argument, deferring all value validation to update_topo() (fixed
  in the previous commit) or to an explicit check_grid() call that the
  caller might never make. Reject a negative value immediately in the
  setter itself -- the only bound it can check locally, since the upper
  bound (dim_topo for positions, n_sub for substation ids) depends on
  context the setter does not have (other containers' element counts /
  SubstationContainer, respectively).

* OneSideContainer::update_topo: a substation id feeds
  SubstationContainer::local_to_gridmodel's unchecked arithmetic
  (sub_id + (busbar - 1) * n_sub) to resolve the target bus. That
  function's own bound check is on the OUTPUT (the resolved bus id, which
  is what actually gets used downstream), not on sub_id -- so an
  out-of-range sub_id can land BY COINCIDENCE on another substation's
  legitimate bus id and silently reconnect the element to the WRONG bus
  rather than raising (reproduced: load reassigned to an unrelated
  substation's bus, no exception). Validate sub_id against
  substations.nb_sub() where that context is available, mirroring the
  el_pos check added for pos_topo_vect in the previous commit; also guard
  the case where subid_ was never set at all (empty), which indexed an
  empty vector.

Tests: 7 new python cases (test_state_poisoning.py) and 4 new C++ cases
(test_check_grid.cpp), all reproduced as failing before the fix and
passing after. Full suites green: 132 C++ tests, 46 python tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Vxxj4GukTFz1TKh52GZxLP
Signed-off-by: Claude <noreply@anthropic.com>
Co-authored-by: DONNOT Benjamin <benjamin.donnot@rte-france.com>
BDonnot added a commit that referenced this pull request Aug 3, 2026
@BDonnot
Merge pull request #161 from Grid2op/claude/security-audit-review-jghnx0
87f6ab2 
Close four more memory-safety holes on the untrusted-input / public-API paths
@BDonnot BDonnot mentioned this pull request Aug 3, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BDonnot @claude