Restart with ns.gradp_in_checkpoint=0 calls computeGradP on an undefined Gradp_Type state

[WeiqunZhang]

Severity: high · Category: correctness, memory-ub · Fix order: 3 of 21 — fix this 3rd.

_{Filenames are numbered in reverse fix order: 001 = fix last, 021 = fix first. This file is 019.}

Location: Source/NavierStokesBase.cpp:2722

Based on commit 9bf664bf (line numbers refer to that tree).

The two findings below are the same defect from two sides: the null-StateDescriptor dereference/abort, and the fact that it is a regression introduced when the restart path was consolidated, which makes the documented "recompute GradP from Pressure" recovery unusable.

The defect

What is wrong — When ns.gradp_in_checkpoint=0, AmrLevel::restart leaves state[Gradp_Type] default-constructed (desc==nullptr, new_data==nullptr), yet restart() immediately calls computeGradP(), which does get_data(Gradp_Type,...) -> StateData::curTime() -> desc->timeType(); nothing ever defines that StateData. _{Reported independently at this same line by F012; each reviewer's own wording and evidence is under Verification evidence below.}

Why it matters

Restart from a pre-Gradp checkpoint (ndesc>nstate) with ns.gradp_in_checkpoint=0 ns.avg_in_checkpoint=1. set_state_in_checkpoint zeroes state_in_checkpoint[Gradp_Type], AmrLevel::restart skips state[Gradp_Type].restart(), then line 2722 dereferences the null StateDescriptor -> segfault (or 'get_data: invalid time' abort). The documented recovery path never runs.

How to reach it

• Restart any deck (e.g. Exec/run2d) from a pre-Dec-2020 checkpoint lacking Gradp_Type: amr.restart=chk_old ns.gradp_in_checkpoint=0 ns.avg_in_checkpoint=1. Keys are live (NavierStokesBase.cpp:538-539); the line-2690 Abort fires only if keys are unset.
• Any deck (e.g. Exec/run2d) with amr.restart=<checkpoint written before Nov 2020 commit d40c8a3, lacking Gradp_Type>, ns.gradp_in_checkpoint=0, ns.avg_in_checkpoint=0 — the exact keys the line-2690 Abort message instructs users to set.

Suggested fix

Restore the state[Gradp_Type].define(...) / allocOldData() that commit 3005a78 (#144) deleted while consolidating the constructor and restart paths; nothing replaced it, and AmrLevel::restart intentionally skips state[i].restart() when state_in_checkpoint[i]==0 (AMReX_AmrLevel.cpp:452), so this path must define the StateData itself — exactly as the sibling recovery for Average_Type in post_restart() (NavierStokesBase.cpp:2486) still does, including passing Factory() so EB runs get an EB-aware MultiFab.

Keep the original time arguments: Gradp_Type is registered StateDescriptor::Interval (NS_setup.cpp:346), and StateData::define then sets new_time=[t,t+dt], old_time=[t-dt,t]. Using t = 0.5*(prevTime+curTime) and dt = curTime-prevTime from Press_Type reproduces Press's own curTime()/prevTime() exactly, so both get_data(Gradp_Type,time) lookups inside computeGradP resolve instead of aborting. Order matters: define, computeGradP(cur_time), allocOldData(), computeGradP(prev_time).

Maintainer decisions: whether to keep this inline in restart() (before define_workspace(), as pre-#144) or factor a shared "define a state type missing from the checkpoint" helper used by both the Gradp and Average recovery paths — worth coordinating with the separate ns.avg_in_checkpoint finding if that one restructures the same code. Either way please add a CI restart test from a checkpoint lacking Gradp_Type; the absence of one is why this sat broken since 2023.

For Source/NavierStokesBase.cpp:2722 (F004):

--- a/Source/NavierStokesBase.cpp
+++ b/Source/NavierStokesBase.cpp
@@ -2716,11 +2716,20 @@
       Print()<<"WARNING! GradP not found in checkpoint file. Recomputing from Pressure."
              <<std::endl;
 
       //
-      // Compute GradP from the Pressure
+      // AmrLevel::restart left state[Gradp_Type] undefined because it was
+      // not in the checkpoint; define it, then compute GradP from the Pressure.
       //
-      computeGradP(state[Press_Type].curTime());
-      computeGradP(state[Press_Type].prevTime());
+      Real cur_time  = state[Press_Type].curTime();
+      Real prev_time = state[Press_Type].prevTime();
+      Real dt = cur_time - prev_time;
+      state[Gradp_Type].define(geom.Domain(), grids, dmap, desc_lst[Gradp_Type],
+                               cur_time, dt, Factory());
+      computeGradP(cur_time);
+
+      // now allocate the old data and fill
+      state[Gradp_Type].allocOldData();
+      computeGradP(prev_time);
     }
 
     define_workspace();

_{Restores the exact pre-#144 code (commit 537438f) that PR #144 dropped: define state[Gradp_Type] (StateData::define with Press curTime and dt=cur-prev, matching the Average_Type recovery in post_restart), computeGradP(cur), allocOldData(), computeGradP(prev). AmrLevel::restart skips StateData::restart when state_in_checkpoint[Gradp_Type]==0, leaving desc null.}

_{Diff(s) are against 9bf664bf, written from the current source and verified only with git apply --check — never compiled, never run, never applied to the tree. Treat them as precise intent, not tested patches.}

Verification evidence

F004 — confirmed (two independent verifier lenses)

Lens 1 (refutation attempt): restart() calls AmrLevel::restart then immediately computeGradP(state[Press_Type].curTime()) at 2722. amrex AMReX_AmrLevel.cpp:452 only calls state[i].restart() 'if (state_in_checkpoint[i])'; skipped StateData stays default-constructed with desc{nullptr} (AMReX_StateData.H:468). computeGradP does get_data(Gradp_Type,time) -> prevTime() -> desc->timeType() -> null deref. Nothing defines state[Gradp_Type] (contrast Average path, 2486 which does define()).

Lens 2 (reachability/intent): Commit 3005a78 (PR #144, 2023) deleted state[Gradp_Type].define(...) and allocOldData() from NSB::restart, keeping only computeGradP(state[Press_Type].curTime()) (line 2722). AmrLevel::restart (AMReX_AmrLevel.cpp:452) skips undefined states; StateData default desc{nullptr} (AMReX_StateData.H:468); get_data calls state[i].prevTime() -> desc->timeType() -> null deref. Original commit 537438f had the define; Average_Type recovery (post_restart:2486) still keeps its define — removal was accidental, not deliberate.

F012 — confirmed (two independent verifier lenses)

Reported as: Restart with ns.gradp_in_checkpoint=0 calls computeGradP on a never-defined StateData: current AMReX AmrLevel::restart leaves states with state_in_checkpoint[i]==0 default-constructed, and commit 3005a78 removed the state[Gradp_Type].define()/allocOldData() that this path requires.

Failure scenario: Restart any pre-2021 checkpoint lacking Gradp_Type with ns.gradp_in_checkpoint=0 (the documented workflow). state[Gradp_Type] has INVALID_TIME and null data, so computeGradP's get_data(Gradp_Type,time) hits amrex::Error("get_data: invalid time") and the run aborts; old-format checkpoints cannot be restarted at all.

Lens 1 (refutation attempt): Confirmed commit 3005a78 ('Consolidate duplicate code between NSB constructor and restart') removed 'state[Gradp_Type].define(geom.Domain(),grids,dmap,desc_lst[Gradp_Type],time,dt,Factory())' and allocOldData() from this path, leaving bare computeGradP(curTime/prevTime) on a default-constructed StateData (desc==nullptr). Old-format checkpoints (ndesc>nstate) with ns.gradp_in_checkpoint=0 crash; the documented recovery path is dead.

Lens 2 (reachability/intent): Commit 3005a78 deleted state[Gradp_Type].define()/allocOldData() from NavierStokesBase::restart while keeping computeGradP(state[Press_Type].curTime()/prevTime()) (NavierStokesBase.cpp:2714-2723). AMReX AmrLevel::restart skips state[i].restart when state_in_checkpoint[i]==0 (AMReX_AmrLevel.cpp:452), leaving default-constructed StateData with INVALID_TIME; computeGradP's get_data(Gradp_Type,time) (line 4116) then hits amrex::Error("get_data: invalid time"). The Average_Type analog (post_restart:2486-2491) still defines before use, confirming the removal was an unintended regression, unfixed on development.

---

_{Based on commit 9bf664bf, which is also the tree the audit verified against. From an automated audit of Source/, Tutorials/ and Util/. Audit finding ids: F004, F012. Reviewer unit(s): NSB-2 init/sync/post-step, theme:restart-regrid. Nothing here was compiled or run — the failure scenarios are code reasoning, so the reaching configuration above is the cheapest way to confirm or refute it.}