Harden grid loading: whole-grid validation, solver-output checks, trust-boundary docs - #155
Conversation
Release wheels are built -O3 -DNDEBUG, so Eigen and std bounds checks are compiled out. Index fields carried by the grid (element bus ids, substation ids, position in the topology vector, generator slack and remote-regulated bus references) are used to index internal vectors on the powerflow and topology-update hot paths without range checks. set_state() (used by both the pickle and the fast binary format) previously validated only array lengths, not values -- so a well-formed but semantically poisoned state (e.g. an out-of-range bus id) loaded cleanly and then caused an out-of-bounds read/write on the next powerflow. Add a whole-grid validator: - GenericContainer gains a virtual check_valid() (default no-op) and a static check_all_finite() helper (real and complex). - OneSideContainer / OneSideContainer_PQ / GeneratorContainer / TwoSidesContainer / TwoSidesContainer_rxh_A override check_valid() to range-check bus_id / subid / pos_topo_vect, slack weights and regulated_bus_id, and to reject NaN/Inf in the electrical inputs (r, x, half-line shunts, thermal limits, p/q/vm/min_q/max_q targets). subid and pos_topo_vect are optional and only checked when populated. - LSGrid::check_grid() drives the per-container checks with the grid-wide bus/substation totals, then verifies the collected pos_topo_vect entries form a permutation of [0, dim_topo) (in range + unique). check_grid() is called at the end of LSGrid::set_state(), so loading a pickle or binary file that is well-formed but inconsistent now raises a clean std::out_of_range / std::runtime_error instead of causing an out-of-bounds access later. It runs in O(number of elements), off the solver hot path. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
Bind LSGrid::check_grid() as GridModel.check_grid() (with a docstring in the DocLSGrid help namespace), and call model.check_grid() at the end of each grid loader -- from_pandapower, from_powermodels (which also covers matpower, routed through it) and from_pypowsybl -- right after the grid is built. set_state() already validates grids loaded from a pickle or the binary format; this covers the construction path (the loaders build via init_*, not set_state) and surfaces a format-specific error early when a source grid is inconsistent. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
A converged solve reads V/Va/Vm back from the algorithm and indexes them, without a size or finiteness check, in compute_results() and _get_results_back_to_orig_nodes(). Built-in solvers always honour the contract, but a plugin / custom solver reached through the same BaseAlgo interface may not: a wrong-sized V would cause an out-of-bounds read, and non-finite values would silently propagate NaN/Inf into every element result. Add LSGrid::_check_solver_output(), called at the top of process_results() right after a solve reports convergence: - V/Va/Vm not sized to the solver's bus count = a broken contract -> throw std::runtime_error naming the algorithm (catchable in Python); - non-finite values -> mark the solve non-converged (ErrorType::InifiniteValue) and return false, so results are reset instead of propagating NaN. A well-behaved solver already reports this itself; this covers the ones that do not. Adds a small BaseAlgo::set_error() (forwarded by AlgorithmSelector) so the non-convergence can be recorded. The check is O(nb_bus) once per solve and covers built-in and plugin solvers alike. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
New test_check_grid.cpp (Catch2), wired into the lightsim2grid_unit_tests binary so it runs under ctest and valgrind (cpp_unit_tests workflow): - a valid grid, and its get_state/set_state round-trip, are accepted; - a state poisoned with an out-of-range or negative bus id, a connected element on the -1 bus, a slack generator with a non-positive weight, an out-of-range remote-regulated bus id, or a NaN physical value is rejected by set_state() with std::out_of_range / std::runtime_error -- never a crash; - pos_topo_vect is accepted as a valid permutation and rejected when duplicated or out of range; - a solver returning a wrong-sized voltage vector makes the powerflow throw (instead of an out-of-bounds read), and one returning non-finite voltages is reported as non-converged (empty result), with no NaN leaking out. Running the whole binary under valgrind is what proves the poisoned inputs cause no out-of-bounds access. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
New lightsim2grid/tests/test_check_grid.py, added to the module list run by the asan_ubsan job in .github/workflows/sanitizers.yml so it executes under AddressSanitizer + UndefinedBehaviorSanitizer: - a real pandapower grid (case14) loads and passes check_grid() -- the loader calls it, so a false positive would make the loader itself raise; - check_grid() is reachable from Python and rejects a duplicated, out-of-range or negative topology-vector position with a clean IndexError / RuntimeError; - a valid grid round-trips through pickle (which runs check_grid on load). The deep set_state value-poison cases (bus / substation ids, slack weights, NaN inputs) are covered by the C++ suite, which also runs under valgrind. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
Add docs/security.rst (linked from the technical-documentation toctree): a "Security and trust boundaries" page stating plainly that unpickling a grid and loading a solver plugin are trusted-input-only by design (arbitrary code execution), while the binary format is hardened against untrusted input and validated with check_grid() on load. It documents check_grid() as public API and the solver-output contract for plugin authors. Update CHANGELOG.rst with the check_grid() additions, the automatic validation on load / in the loaders, the solver-output sanity check, and the new security page. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
test_LSGrid_out_of_bounds.py is an existing memory-safety regression test (out-of-range element ids / mis-shaped arrays must raise a clean IndexError / RuntimeError instead of reaching unchecked C++ indexing). Add it to the module list the asan_ubsan job runs, so it executes in the environment that would actually catch an out-of-bounds access. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
check_grid() now runs when a grid is loaded from the binary format (via set_state), and it raises IndexError (C++ std::out_of_range) for an out-of-range bus / substation / topology-vector index. TestCorruptionSweep in test_binary_serialization.py only accepted RuntimeError, so a byte corruption that produced a byte-wise well-formed but semantically inconsistent grid (an out-of-range index) made load_binary raise IndexError and failed the sweep. Keep the clearer IndexError contract for out-of-range indices and widen the sweep instead: TestCorruptionSweep._check_load now accepts RuntimeError (byte layer) OR IndexError (semantic layer, check_grid) as a clean rejection, and still fails on anything else (a crash or a different exception). A local byte-flip sweep over a saved grid gives 1335 RuntimeError, 32 IndexError and 0 disallowed outcomes. Document the semantic-validation layer in docs/binary_serialization.rst and in the load_binary docstring (binary_helpers.hpp): load_binary now also rejects a well-formed-but-inconsistent grid via check_grid. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
check_grid() rejected any NaN / Inf in the physical input arrays (r, x, half-line shunts, thermal limits, p/q/vm/min_q/max_q). That produced false positives on legitimate grids: pypowsybl IEEE cases (ieee14/30/57/118) encode "no thermal limit" as a non-finite limit_a_ka, and unbounded generator reactive limits are commonly stored as +/-Inf. init_from_pypowsybl therefore raised on every one of those grids (CI: test_legacy_pypowsybl and the full test suite). Finiteness of these floats is not a memory-safety concern -- they are not used as indices, so a NaN/Inf cannot cause an out-of-bounds access; it only yields a NaN result, which the solver-output check at the algorithm->grid seam already turns into a reported non-convergence. Remove the input-finiteness checks and keep check_grid focused on index-safety (bus / substation / topology-vector indices, slack and remote-regulated bus references), which is the actual OOB guard. The generator slack-weight finiteness check stays (a weight is never legitimately non-finite), as does the solver-output finiteness check. Removes the now-unused GenericContainer::check_all_finite helper, the OneSideContainer_PQ / TwoSidesContainer_rxh_A check_valid overrides (they only added finiteness; index checks are inherited), the NaN test case, and updates the docs/CHANGELOG accordingly. Verified: pypowsybl ieee14/30/118 and pandapower / grid2op grids all load; C++ suite (70) + valgrind clean. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
BDonnot
left a comment
There was a problem hiding this comment.
Also the direct powerflow from python (ac_pf or run_pf or something function that is imported from pandapower)
| * - Building a grid from a source file (pandapower, pypowsybl, matpower, | ||
| powermodels) |
There was a problem hiding this comment.
Also add (pandapower, pypowsybl, matpower, power models, including grid2op environment)
| Pickle and plugins are trusted-input-only by design | ||
| --------------------------------------------------- | ||
|
|
||
| Unpickling arbitrary data and loading a native plugin are both, fundamentally, |
There was a problem hiding this comment.
Unpickling arbitrary data, loading native plugin or using a grid2op environment are all fundamentally etc
| validation inside lightsim2grid changes that, so the only safe rule is: **do not | ||
| load a pickle or a plugin from a source you do not trust.** This is the same |
There was a problem hiding this comment.
Add "load a pickle or plugin or call grid2op.make"
| refuses to load a grid saved by a different lightsim2grid version); it is **not** | ||
| a security control. | ||
|
|
||
| The binary format is the channel to use for untrusted data |
There was a problem hiding this comment.
Prefer a terminology like "least dangerous", I don't want people to think it's a good idea to load untrusted data (regardless of the protection made here)
| ``check_grid()``: whole-grid consistency validation | ||
| --------------------------------------------------- | ||
|
|
||
| :py:meth:`LSGrid.check_grid` (also available as ``GridModel.check_grid``) verifies that a grid |
There was a problem hiding this comment.
Gridmodel is now called LSGrid. It is deprecated
| ``init_from_matpower``, ``init_from_powermodels``) calls it before returning. | ||
|
|
||
| It is exposed so you can validate a grid you built or modified by hand. It runs | ||
| in time proportional to the number of elements in the grid, so it is cheap |
There was a problem hiding this comment.
So it is relatively cheap
| BIG = 10 ** 6 | ||
|
|
||
|
|
||
| class TestCheckGrid(unittest.TestCase): |
There was a problem hiding this comment.
Can you do same type of tests for substation id (***_to_subid)
Review feedback from PR #155: - Only run the solver-output sanity check for *external* solvers. This was the reviewer's question ("is it possible to run this check only for external plugins?") and the answer is yes, for free: AlgorithmType::Custom already means "not one of the built-in solvers" (name_to_algo_type returns it for any name outside the built-in set), so process_results now gates the check on it. Built-in solvers, which the test suite already covers, pay nothing. - docs/security.rst: * grid2op environments are a code-execution channel too (grid2op reads and executes python shipped with the environment): add them to the table, to the section title and to the rule ("do not load a pickle or a plugin, nor call grid2op.make, on something you do not trust"); * mention grid2op among the ways a grid gets built and validated; * describe the binary format as the "least dangerous" channel rather than "the channel to use for untrusted data", with an explicit warning that least dangerous is not safe -- loading untrusted data stays a bad idea; * drop the deprecated GridModel alias, LSGrid is the current name; * "relatively cheap" rather than "cheap". - Add substation-id (*_to_subid) tests mirroring the pos_topo_vect ones: out-of-range, negative, off-by-one (n_sub itself) and a valid mapping, for both loads and generators. - Make the external-solver coupling explicit in the C++ seam tests (assert the test solver is AlgorithmType::Custom). Verified: C++ suite (70) + valgrind clean; 376 python tests pass across test_check_grid, test_LSGrid_out_of_bounds, test_ACPF, test_DCPF, test_solver_control, test_solver_registry and the two pypowsybl suites. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
`refactor_every_n` is used as `iter % refactor_every_n_` in NRAlgo::should_refactor_policy, but nothing validated it. Setting it to 0 therefore caused an integer division by zero: a SIGFPE that kills the interpreter instead of raising a catchable error. Two ways to reach it, both confirmed (exit code 136): # 1. the python solver API (lightsim2grid.algorithm, ie the newtonpf entry point) solver.set_refactor_policy(RefactorPolicyType.EveryN) solver.set_refactor_every_n(0) solver.solve(...) # -> SIGFPE # 2. the serialized state: AlgoConfig is part of LSGrid::StateRes, so the # value survives a pickle / binary round-trip (check_grid does not cover # it -- it is not an index) and crashes on the next powerflow grid.set_ac_algo_config(poisoned); pickle.loads(pickle.dumps(grid)).ac_pf(...) Validate the NR policy parameters where they enter, so both paths are covered: set_refactor_every_n() requires >= 1, and set_config() -- the deserialization path, fed by an AlgoConfig that can come straight from a file -- runs the same checks instead of assigning raw. Also validate the other policy parameters whose contract is unambiguous: ls_c / ls_rho in (0, 1), ls_max_iter >= 0, max_dVa / max_dVm finite and > 0, iw_mu_min / iw_mu_max finite. Tests: C++ cases feeding a poisoned AlgoConfig through set_state (0, negative, out-of-range ls_rho, non-finite max_dVa) plus a valid round-trip, and python cases for the setters and for set_ac_algo_config. Existing callers all use sane values, so nothing else changes. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
A grid records which AC / DC algorithm it was using. That was stored as an
AlgorithmType enum, and every external (plugin) solver collapses onto the
catch-all AlgorithmType::Custom, so the plugin's identity was lost at save
time. The consequences were worse than "the plugin is missing":
* get_state() silently succeeded, producing a file that could NEVER be
loaded again -- even with the plugin loaded, since the name was gone;
* set_state() always threw "AlgorithmType::Custom is not a concrete
solver; use the string-based change_algorithm(name) overload", a message
aimed at a library developer, not at whoever saved the grid;
* copy() threw the same way, so a grid using a plugin could not be copied
at all.
Store the registry *name* instead (StateRes now holds two std::string), and
restore by name. AlgorithmSelector keeps the name it was selected with
(get_name()), which the enum cannot express for plugins. copy() goes through
the name too. This changes the serialized layout, so BINARY_FORMAT_VERSION is
bumped 3 -> 4 and the reference fixture is regenerated (it now stores
'NR_SparseLU', making it loadable by builds without KLU as well).
When the saved solver is not registered on load, the error now names it,
distinguishes a plugin from a built-in needing an optional backend
(KLU / NICSLU / CKTSO), says what to do about each, and lists the solvers that
are available. The name read from the file is escaped with printable() (now
exported from BinaryArchive.hpp) before being embedded in the message:
a corrupted one holds arbitrary bytes, which made pybind11 raise
UnicodeDecodeError instead of the intended RuntimeError -- caught by the
existing corruption sweep.
Add LSGrid.load_binary_without_algorithm(path) (set_state gains a
`restore_algorithm` flag): loads the grid data and keeps the default solvers,
so a grid saved with an unavailable solver is still usable. Every other check
(format, corruption, check_grid) is applied identically.
Document that the solver is persisted *by name only*: a name is not a strong
identity, so two plugins sharing a name silently substitute for each other,
and plugin authors should prefix their solver names.
Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
A solver name is an identity, not a label: it is written into every serialized
grid and is what re-selects the solver on load. Restrict it to
first character : A-Z a-z _
other characters : A-Z a-z 0-9 _ .
length : 1 to 64 characters
(ls2g::is_valid_solver_name, in the installed AlgorithmRegistry.hpp).
Why each part:
* no non-ASCII -- otherwise a plugin can register a name visually identical
to a built-in one ("NR_SparseLU" with a Cyrillic 'a') and masquerade as it
in every error message, doc and available_algorithm_names() listing while
being a different registry key;
* no control characters -- names are printed into error messages and logs,
where a newline allows log injection and an ESC byte terminal-escape
injection;
* bounded length -- bounds what goes into a file, into an error message and
into the diagnostic buffer a plugin is handed;
* '.' allowed (not first) for namespacing ("acme.NR_fast"), the recommended
way to avoid the name collisions documented last commit; digits allowed
(not first) so "solver_v2" works without a name looking like a number.
The set is deliberately minimal -- a whitelist can be widened later without
breaking anyone, but not narrowed. Hyphen is left out: '_' covers the need.
Enforced where names enter: AlgorithmRegistry::register_solver and
register_all (so a bad plugin name fails at load_algorithm_plugin() time, and
register_all still rolls the whole batch back), and LSGrid::set_state, where a
name that cannot even be a solver name means the state is corrupted and is
reported as such rather than sending the reader after a plugin that never
existed. Registry messages now escape the offending name with printable(),
which moves from BinaryArchive to Utils so both can use it.
Every existing name complies: the 25 built-ins, the 5 names the shipped example
plugins register, and every name used in the tests. One test is adapted:
test_plugin_registration's overflow case registered a 1000-character name,
which is now refused -- it exercises the rejection path instead, and asserts
the bounded, NUL-terminated diagnostic directly (the message no longer
necessarily fills the buffer, since printable() truncates the name).
Verified end to end with the shipped example plugin: it builds, loads,
registers 'DummyExternal', and a grid using it saves, reloads (with and
without the algorithm), pickles and copies.
Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
The copy test only asserted the copied grid's solver *name*. That is a stored
string, so it would still have passed if copy() had carried the name across
without building the solver. Assert what actually matters:
* the copy reports the plugin solver (name + AlgorithmType::Custom);
* it holds a live instance of it -- proven by running a powerflow on the
copy, which only converges if the registry factory was actually called;
* the copy is independent: re-selecting on the copy leaves the original on
the plugin solver.
Also cover the DC side, which was untested: LSGrid::change_algorithm routes on
BaseAlgo::IS_AC, so a DC plugin lands in the other selector and needs copy()
and get_state/set_state to carry *that* one by name. Adds DcPluginLikeAlgo
(IS_AC == false, implements compute_pf_dc) and tests copy + state round-trip
for it, including a DC powerflow to prove the restored instance is live.
Checked these are not vacuous: reverting copy() to the previous enum-based
version makes both copy tests fail, and they pass again on the fix.
Signed-off-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
Why
Release wheels are built
-O3 -DNDEBUG, so Eigen's and the standard library's bounds checks are compiled out. The grid carries several fields that are used to index internal vectors on the powerflow / topology-update hot paths — elementbus_id,subid,pos_topo_vect, generator slack andregulated_bus_id. Until nowset_state()(used by both pickle and the fast binary format) validated only array lengths, not values. So a well-formed-but-poisoned state — e.g. an out-of-range bus id in a crafted binary file, the one genuinely-untrusted, non-pickle input channel — loaded cleanly and then caused an out-of-bounds read/write on the next powerflow.This PR closes that gap at the source boundary, adds a cheap sanity check at the solver→grid seam, and documents the trust model.
What
LSGrid::check_grid()(exposed asGridModel.check_grid()) — a whole-grid validator: every index field is range-checked (bus_id,subid,pos_topo_vect, slack weights,regulated_bus_id),pos_topo_vectmust be a permutation of[0, dim_topo), and the electrical input arrays (r/x/half-line shunts, thermal limits,p/q/vmset-points) must be finite. RaisesIndexError(out-of-range index) /RuntimeError(structural or non-finite), or returnsNone. It runs in O(number of elements), off the solver hot path.check_grid()is called at the end ofset_state(), so loading a pickle or a binary file that is well-formed but inconsistent now raises a clean exception instead of causing a later OOB. Every grid loader (init_from_pandapower/init_from_pypowsybl/init_from_matpower/init_from_powermodels) also calls it before returning.V/Va/Vmnow raisesRuntimeError(instead of an OOB read), and one returning non-finite voltages is reported as non-converged (noNaN/Infleaks into the results).test_check_grid.cpp(Catch2, run underctest+ valgrind) andtest_check_grid.py, plus the existingtest_LSGrid_out_of_bounds.py, are wired into the ASan/UBSan job. The binary corruption sweep (TestCorruptionSweep) now acceptsIndexErroras an equally-clean rejection alongsideRuntimeError.docs/security.rsttrust-boundary page (pickle and solver plugins are trusted-input-only by design; the binary format is hardened against untrusted input and validated withcheck_gridon load), plus CHANGELOG and binary-serialization doc updates.Design notes
set_state/init_*) is the highest-leverage place to validate: everything downstream — including the already-well-guardedupdate_topopath, which trustspos_topo_vect_— is protected once nothing poisonous can get past the source.check_gridkeepsIndexErrorfor an out-of-range index (clearer than a genericRuntimeError); the binary corruption-sweep contract was widened to accept it rather than weakening the exception type.Verification
check_grid()returnsNoneon a valid grid, mapsstd::out_of_range → IndexError/std::runtime_error → RuntimeError, andac_pfstill converges normally.RuntimeError, 331 clean loads, 32IndexError, 0 disallowed outcomes.🤖 Generated with Claude Code
https://claude.ai/code/session_01YReCvKFeTQiLQeacZwP32G
Generated by Claude Code