Skip to content

fix(fp): bound fp_get_state copies by the allocated extents (#245) - #248

Merged
k-yoshimi merged 1 commit into
developfrom
fix/fp-get-state-extents-245
Oct 13, 2026
Merged

k-yoshimi merged 1 commit into
developfrom
fix/fp-get-state-extents-245

Conversation

@k-yoshimi

@k-yoshimi k-yoshimi commented Oct 13, 2026 •

Copy link
Copy Markdown
Owner

Fixes the pre-existing read past the FPCOMM profile arrays in fp_get_state reported in #245 (found by the Codex review of #242).

Problem

RNT/RWT/RTT/RJT/RPCT/RPWT keep the extents of the last fp_prep (fp_allocate_ntg2 in fp/fpcomm.f90), while NRMAX / NSAMAX are the current namelist values. fp_set_param on either count clears g_prepared without reallocating, so between that call and the next fp_run, fp_get_state copied nsa = 1..NSAMAX, nr = 1..NRMAX out of smaller arrays. The library is built with -O3 and no run-time checks, so the read was silent garbage (an abort under -fcheck=all). #242's early refusal in fp_run added one more way to be left in that window.

Fix (option a of the issue)

fp_get_state copies MIN(NRMAX, SIZE(RNT,1)) x MIN(NSAMAX, SIZE(RNT,2)) (and ntg_last bounded by SIZE(RNT,3)) and reports the copied extents in state%nrmax / state%nsamax, so the [nsamax][nrmax] slice is always data that exists: a lowered count truncates the snapshot at once, a raised one stays capped at the old allocation until the next run. Unchanged: the pre-first-run path (nothing allocated: namelist counts, zero profiles) and the rc=3 refusal above FP_MAX_NRMAX. The C ABI layout is unchanged; no consumer (fplib/state.py, fp_mcp, totlib/pipeline.py, extract_fp_metrics.py, fpregress.f90) indexes beyond the reported counts.

Tests

python/fplib/tests/test_state_extents.py (9 cases, one per case, no subTest): NSAMAX / NRMAX raised without a run (the two reads past the arrays), NRMAX / NSAMAX lowered, NRMAX raised then run (the bound follows the reallocation), NRMAX above the cap still rc=3, fp_run(0), a second handle in the same process reading before its first run (per-process arrays, counts reset by fp_init: a 1 x 1 slice of the old data), and the pre-first-run snapshot; all six profile arrays are checked. The assertions are on the reported extents and copied values, so they hold with or without bounds checking.

Verified once with fp_api.o compiled with DFLAGS (-fcheck=all): develop's fp_api.f90 fails the two raised-count cases, this branch passes. Local run with the CI flags (--forked --timeout=120 --timeout-method=signal): fplib + fp_mcp + totlib = 278 passed, 1 pre-existing macOS/py3.10 failure (ExceptionGroup), 18 pre-existing skips.

Docs

docs/fp-library/architecture.md (the "do not call fp_get_state in that window" note from #242 replaced by the new contract, incl. the per-process allocation), docs/sphinx/modules/fp/{en,ja}/state.md.

Review

In-house review + Codex (codex-rescue) + ai-review-cycle Codex pass on the diff: no HIGH; one MED (docs named a nonexistent fp_get_param) and the wording LOWs are fixed in this commit; the extra test cases they asked for are included.

Note for #233: its insertion after the profile loop should merge cleanly (pure insertion, 8 unchanged lines below the last changed line); its own P*T loops already bound by SIZE, which is the pattern this PR adopts for the profiles.

Refs: #245, #242, #233, #143.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TA2KahTs3qLMZL6yjSsWwT


Note

Medium Risk
Changes C ABI snapshot semantics for fp_get_state when mesh/species counts disagree with allocation; callers that assumed nrmax/nsamax always matched namelist values could misread results, though the fix prevents silent OOB reads.

Overview
Fixes fp_get_state reading past allocated FPCOMM profile arrays when NRMAX or NSAMAX change via fp_set_param before the next fp_run (#245). The copy loops now use MIN(current count, allocated SIZE) on each axis, set state.nrmax / state.nsamax to the copied extents, and cap the time index the same way—so returned [nsamax][nrmax] data is always in-bounds (truncate on lower counts, cap on raised counts until reallocation).

Unchanged behavior is documented and tested: pre-first-run namelist counts with zero profiles, rc=3 above FP_MAX_NRMAX, and per-process array lifetime (second handle may see a truncated slice of prior data). Docs in architecture.md and Sphinx state.md (EN/JA) replace the old “do not call fp_get_state in that window” note with this contract. New python/fplib/tests/test_state_extents.py covers raised/lowered counts, post-run reallocation, zero-step run, and edge cases.

Reviewed by Cursor Bugbot for commit f7d8c2d. Bugbot is set up for automated code reviews on this repo. Configure here.

The FPCOMM profile arrays (RNT, RWT, RTT, RJT, RPCT, RPWT) keep the extents
of the last fp_prep, while NRMAX / NSAMAX are the current namelist values.
fp_set_param on either count clears g_prepared without reallocating, so
until the next fp_run the two disagree and fp_get_state copied
nsa = 1..NSAMAX, nr = 1..NRMAX out of smaller arrays: a read past the
allocation (silent in the -O3 library build, an abort under -fcheck=all).
#242's early refusal in fp_run added one more way to be left in that
window.

fp_get_state now copies MIN(NRMAX, SIZE(RNT,1)) x MIN(NSAMAX, SIZE(RNT,2))
(and ntg_last bounded by SIZE(RNT,3)) and reports the copied extents in
state%nrmax / state%nsamax, so the [nsamax][nrmax] slice is always data
that exists: a lowered count truncates the snapshot at once, a raised one
stays capped at the old allocation until the next run. Behaviour before
the first run in a process (nothing allocated: namelist counts, zero
profiles) and the rc=3 refusal above FP_MAX_NRMAX are unchanged.

Tests: python/fplib/tests/test_state_extents.py (9 cases, no subTest):
NSAMAX / NRMAX raised without a run (the two reads past the arrays),
NRMAX / NSAMAX lowered, NRMAX raised then run (the bound follows the
reallocation), NRMAX above the cap still rc=3, fp_run(0), a second handle
in the same process reading before its first run (per-process arrays,
counts reset by fp_init: a 1 x 1 slice of the old data), and the
pre-first-run snapshot; all six profile arrays are checked. The
assertions are on the reported extents and copied values, so they hold
with or without run-time bounds checking. Verified once with fp_api.o
compiled with DFLAGS (-fcheck=all): develop's fp_api.f90 fails the two
raised-count cases, this one passes.

Docs: docs/fp-library/architecture.md (the "do not call fp_get_state in
that window" note from #242 replaced by the new contract, incl. the
per-process allocation) and the fp state.md tables (en/ja).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TA2KahTs3qLMZL6yjSsWwT
@k-yoshimi

Copy link
Copy Markdown
Owner Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f7d8c2d. Configure here.

self.assertEqual(state.timefp, 0.0)
for name, rows in _profiles(state).items():
self.assertEqual(len(rows[0]), 3, name)
self.assertTrue(all(v == 0.0 for row in rows for v in row), name)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pre-run state test depends on order

Medium Severity

test_before_first_run_reports_namelist_counts_and_zeros expects namelist NRMAX and all-zero profiles, which holds only while RNT is unallocated. fp_finalize keeps those arrays, and earlier tests in this module allocate them. In pytest's default single-process order the snapshot is the previous run's leading slice, so the extent and zero checks fail. --forked hides that.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f7d8c2d. Configure here.

@k-yoshimi
k-yoshimi merged commit ec87754 into develop Oct 13, 2026
4 checks passed
HengyuLi-Ozaki-lab added a commit to HengyuLi-Ozaki-lab/task that referenced this pull request Oct 14, 2026
develop gained k-yoshimi#242, k-yoshimi#243, k-yoshimi#248, k-yoshimi#249, k-yoshimi#251, k-yoshimi#254 and k-yoshimi#255 since this
branch left it. k-yoshimi#248 (fp_get_state bounds its copies by the allocated
extents) touches the same declaration line of fp_api_get_state as this
branch.

One conflict, fp/fp_api.f90: the local variables of fp_api_get_state.
Both sides added names to the same INTEGER statement; the merge keeps
all of them (ntg1_last from this branch, nr_alloc and nsa_alloc from
k-yoshimi#248). The two bodies do not overlap: k-yoshimi#248's copy of the six profiles is
bounded by the allocated extents, and the global scalars of this branch
were already bounded by SIZE(PIT, 1) and SIZE(PIT, 2). In the state k-yoshimi#248
is about (a count changed by fp_set_param after a run) the scalar block
is not executed at all: fp_set_param clears g_prepared, and the scalars
read zero, as this branch's documentation says.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HengyuLi-Ozaki-lab added a commit to HengyuLi-Ozaki-lab/task that referenced this pull request Oct 14, 2026
After the merge of develop, fp_get_state follows two rules in the window
between an fp_set_param and the next run: the six profiles are still the
last run's, within the allocated extents (k-yoshimi#245, k-yoshimi#248), and the seven
global scalars read zero, because they are filled from a prepared state
only. Nothing tested the second in that window; a change of the gate
would have left the suite green.

python/fplib/tests/test_state_scalars_window.py, 8 cases: the state has
the seven; zero before the first run; the volume and the stored energy
after a run, and after a run of zero steps; after a set_param of
NSAMAX + 1, NRMAX + 1 and NRMAX - 1 the seven are zero while timefp and
the profiles are the last run's within the extents k-yoshimi#245 gives (3 x 1,
3 x 1, 2 x 1); the next run fills them again. With the gate changed to
"the arrays are allocated" the three window cases and the last one fail
(measured).

The documents that said "zero until the first run" now also say "and
again after a set_param until the next run that is made, while the
profiles are still those of the last run" (architecture.md, state.md en
and ja, the fplib README, fp_api.h, state.py, the fp_mcp schema text, a
comment in fp_api.f90). "That is made": since k-yoshimi#242 an fp_run can be
refused before it prepares.

CHANGELOG.md gets the entry this pull request did not have.

No code change: the comment in fp_api.f90 is the only Fortran line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HengyuLi-Ozaki-lab added a commit to HengyuLi-Ozaki-lab/task that referenced this pull request Oct 18, 2026
develop gained k-yoshimi#242, k-yoshimi#243, k-yoshimi#248, k-yoshimi#249, k-yoshimi#251, k-yoshimi#254, k-yoshimi#255, k-yoshimi#256, k-yoshimi#257,
k-yoshimi#258, k-yoshimi#259, k-yoshimi#261, k-yoshimi#263 and k-yoshimi#264 since this branch left it.

One conflict, in python/eqlib/_ffi.py, python/fplib/_ffi.py and
python/trlib/_ffi.py alike: the end of load_library. k-yoshimi#261 (k-yoshimi#246) checks
the state layout of the library just loaded and raises
<Mod>libAbiError on a mismatch; this branch closes the directory it
registered with os.add_dll_directory when the load fails. The check is
now inside the block that cleans up, so a library refused for its
layout does not leave its directory in the process-wide DLL search set
either (or an entry in _DLL_DIR_HANDLES). Order as on develop: the
layout check, then the prototypes.

With the check on the load path, the cases of test_platform_libname.py
that fake the loaded library (ctypes.CDLL patched) were refused by it:
24 of 39 failed. Each of the three files now has an autouse fixture that
patches check_state_layout out for them (the check has its own cases in
test_ffi.py, TestLibraryLayout), and one new case per module puts a
check back that raises AbiMismatchError (with the prototypes passing,
so that only the check can refuse the load) and asserts
<Mod>libAbiError, the directory handle closed and nothing registered;
with the check moved after the cleanup block, that case fails (measured
on eq).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HengyuLi-Ozaki-lab added a commit to HengyuLi-Ozaki-lab/task that referenced this pull request Oct 21, 2026
develop now carries the pieces split out of this branch (k-yoshimi#256 eq_save,
k-yoshimi#257 the MCP fd isolation, k-yoshimi#265 the mcp pin) and k-yoshimi#232, k-yoshimi#233, k-yoshimi#240, k-yoshimi#242,
k-yoshimi#243, k-yoshimi#248, k-yoshimi#249, k-yoshimi#251, k-yoshimi#254, k-yoshimi#255, k-yoshimi#258, k-yoshimi#259, k-yoshimi#261, k-yoshimi#263, k-yoshimi#264. 43
files conflicted; each was resolved as follows.

- Taken from develop (36 files): every file where this branch only
  changed the mcp pin to "mcp>=0.9,<2" (develop has ">=1.2,<2", k-yoshimi#265):
  the seven pyproject.toml, six READMEs, the fp, ti, tot, wr and wrx
  servers, the Sphinx MCP pages, the trlib presentation and its
  builder, the 2026-04-18 MCP plan; and the eq_save pieces now in
  develop in their reviewed form: eq/eq_api.f90, eq/eq_api.h,
  python/eqlib/tests/test_save.py, eq_mcp/tests/test_save_tool.py; and
  tr_mcp/server.py, whose changes here were this branch's own fd
  isolation (develop's _fd_isolation replaces it) and the os._exit(0)
  at the end of main().
- The os._exit(0) that ended main() of tr_mcp and eq_mcp on this branch
  (to skip the Python teardown, cited in k-yoshimi#227 item 5) is gone with
  them: develop's main() returns 0, as approved in k-yoshimi#257. The abort it
  avoided was a double DEALLOCATE, which this branch fixed in
  tr/trcomm.f90 (DEALLOCATE_TRCOMM returns when nothing is allocated);
  that fix stays, merged without a conflict.
- eq/eqfile.f90: develop's (EQSAVE without an argument, EQSAVE_IERR
  doing the work, and its kept EQDSKR error), plus this branch's
  MODELG=25 in EQ_READ's EQDSK branch (k-yoshimi#228-2). eq/eqmenu.f90 merged
  without a conflict but kept this branch's "CALL EQSAVE(IERR)"; it is
  develop's "CALL EQSAVE" again, as EQSAVE takes no argument.
- python/eqlib/_ffi.py, eqlib.py, tests/test_eqlib.py: develop's, plus
  this branch's get_psi_rz (the eq_common_get_psi_rz_ prototype, the
  Eq.get_psi_rz method, its closed-handle assertion).
- eq_mcp/server.py: develop's (the save tool of k-yoshimi#256, the shared fd
  isolation), plus this branch's get_psi_rz handler and tool and its
  --print-tools entry (13 tools). eq_mcp/tests/test_server.py: this
  branch's (13 tools with save and get_psi_rz), which contains
  develop's change.
- python/mcp-servers/eq_mcp/tests/test_import_isolation.py (this branch
  only, no conflict) is removed: it pinned this branch's own isolation
  helper (_install_fd_isolation, _FD_ISOLATION_INSTALLED in eq_mcp and
  tr_mcp), which develop's python/_fd_isolation.py replaces; develop's
  tests/test_fd_isolation.py of each of the seven servers checks the
  same contract (an import leaves fd 1 and sys.stdout alone).
- .github/workflows/python-tests.yml: develop's mcp pin; this branch's
  numpy install (the get_psi_rz tests import numpy) and ../task symlink
  step kept.
- CHANGELOG.md did not conflict. The three k-yoshimi#228 entries this branch held
  back in d8b6e4d are added to [Unreleased] Fixed, after its first
  entry, where no other open pull request inserts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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