ConvertCheckpointGrids: nsets_save OOB write, null old_data deref, 0-ghost interp stencil

[WeiqunZhang]

Severity: high/medium/low · Category: correctness, memory-ub · Fix order: 9 of 21 — fix this 9th.

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

Locations: Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:300, Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:723, Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:672, Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:697

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

The six findings are three defects in one utility: nsets_save bookkeeping, an unguarded old_data, and a coarse interpolation stencil that is both too small and never filled. They share the read/interpolate path and should land as one patch.

The defect

Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:300 — nsets_save (a Vector of size 1, line 69) is written at indices 0..ndesc-1 at level 0, but the resize at line 282 only runs when lev==1 — an out-of-bounds heap write on every run, and the resize never happens for single-level checkpoints. _{Reported independently at this same line by F055; each reviewer's own wording and evidence is under Verification evidence below.}

Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:723 — cell_cons_interp requires the coarse fab to contain coarsen(fine_box) grown by 1 (CellConservativeLinear::CoarseBox), but ngrow_loc is forced to 0 for the trailing state types (lines 660-668), so the slope stencil reads one cell outside the allocated coarse fab.

Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:672 — ConvertData unconditionally dereferences falRef_src.state[n].old_data (here and in the copy at line 698), but old_data is null when the checkpoint stores only one data set (nsets==1), which the reader (lines 309/323) and writer explicitly support. _{Reported independently at this same line by F078; each reviewer's own wording and evidence is under Verification evidence below.}

Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:697 — Source ghost cells at periodic/physical domain boundaries are never filled before interpolation: MultiFab::copy defaults to Periodicity::NonPeriodic and no FillBoundary is called on NewData_src/OldData_src, so domain-edge ghosts keep the setVal(10.) sentinel that feeds the interpolation slopes.

Why it matters

F009: Any IAMR checkpoint has ndesc>=3 (State, Press, Gradp), so lev-0 processing writes nsets_save[1], nsets_save[2],... past the 1-element allocation (heap corruption; abort under ASan/debug). For a finest_level==0 checkpoint the writer also reads nsets_save[i>=1] OOB (lines 505/517/524), possibly VisMF::Write-ing a null old_data.

F010: interp_kind=refine on a checkpoint from a run with divu (5 state types: Dsdt gets ngrow_loc=0) or with Average_Type (sizes 4/6): mf_compute_slopes_x reads u(i-1)/u(i+1) outside the 0-ghost cfab (AMReX_MFInterp_C.H:14) — heap OOB read; garbage values silently written into the converted Dsdt/Average data, or Array4 bounds abort in debug builds.

F045: Run the tool on a chk00000 written by a run with ns.init_iter=0 and check_int>0: post_init_press early-returns at NavierStokes.cpp:1310 when init_iter<=0, so advance_setup never runs and never allocOldData()s the advected states. State_Type and the tracers are therefore checkpointed with nsets==1 (Press_Type and Gradp_Type are the exception -- they get allocOldData at construction, NavierStokesBase.cpp:265-266), and line 672 dereferences a null old_data. With the default ns.init_iter>0 the initial pressure iterations do run advance_setup, every state has nsets==2, and the tool does not crash.

F046: interp_kind=refine on a periodic-domain checkpoint (e.g., Taylor-Green): with default (bogus) BCRec the slope kernel uses central differences through the 10.0 ghost at every domain-edge coarse cell, forcing wrong (sign-flipped/mis-limited) slopes; the refined checkpoint has wrong values in all fine cells along the domain boundary, producing restart artifacts.

How to reach it

• Build Util/ConvertCheckpoint, run ConvertCheckpointGrids2d.gnu.ex checkin=<any IAMR chk> checkout=chk_new user_ratio=2 interp_kind=refine. Level-0 read loop OOB-writes nsets_save[1..ndesc-1] on every run; finest_level==0 checkpoints also OOB-read in the writer.
• Run IAMR with Tutorials/HotSpot/inputs.2d.average_hotspot (ns.do_temp=1, ns.avg_interval=1 gives 6 state types; any test_grids deck gives 5), checkpoint after one step, then run ConvertCheckpointGrids with checkin=chk..., user_ratio=2, interp_kind=refine.

Suggested fix

nsets_save: resize where ndesc becomes known — lev 0, before the descriptor loop. Better, drop the global and let WriteCheckpointFile derive nsets from the pointers as StateData::checkPoint does (dump_old from old_data == nullptr); that also covers single-level files and levels whose nsets differ.

old_data: take ncomps from new_data and skip the OldData allocate/copy/interpolate when old_data is null; the writer's dump_old path already emits nsets=1.

Ghosts: the ngrow_loc=0 cases mirror IAMR's descriptor nExtra (Dsdt/Average are 0), but the copy already passes src_nghost=0, so workspace ghosts are free — and CellConservativeLinear::CoarseBox grows by one while mf_compute_slopes_* reads i±1 under the default bogus BCRec. e4bbe46 added ngrow=1 for this when 556d43b switched pc_interp to cell_cons_interp; 9a52128 undid it for trailing types. One ghost is restart-safe (VisMF::readFAB tolerates an ngrow mismatch). Fill those ghosts with geom.periodicity(): FillBoundary is a no-op at zero ghosts, so F010 and F046 must land together. Non-periodic sides are your call — descriptors are never restored, so real BCRecs are unavailable; FillDomainBoundary with foextrap BCRecs is the closest stand-in.

For Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:300 (F009):

--- a/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
+++ b/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
@@ -279,7 +279,7 @@
       ndesc_save = ndesc;
 
       // ndesc depends on which descriptor so we store a value for each
-      if (lev == 1) nsets_save.resize(ndesc_save);
+      if (lev == 0) nsets_save.resize(ndesc_save);
 
       falRef.state.resize(ndesc);
       falRef.new_state.resize(ndesc);

_{Resize nsets_save as soon as ndesc is known (level 0, before the descriptor loop) instead of at lev==1. Removes the OOB writes at indices 1..ndesc-1 during level-0 reading and makes the writer's nsets_save[i] reads valid for finest_level==0 checkpoints. ndesc_save==ndesc here, so the expression is unchanged.}

For Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:723 (F010):

--- a/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
+++ b/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
@@ -655,17 +655,6 @@
         if(n == 1) ngrow_loc = 1;
       }
 
-      // We don't have the same number of ghost-cells for each data type
-      // Warning, this should be adapted for EB
-      if (falRef_src.state.size() == 4 && n == falRef_src.state.size()-1){
-        ngrow_loc = 0; // For this case, we just have Average_Type and no Divu_Type and Dsdt_type
-      }
-      else if (falRef_src.state.size() == 5 && n == falRef_src.state.size()-1){
-        ngrow_loc = 0; // For this case, we have Divu_Type and Dsdt_type, no Average_Type
-      }
-      else if (falRef_src.state.size() == 6 && n >= falRef_src.state.size()-2){
-        ngrow_loc = 0; // Here we have both Average_Type and Divu and Dsdt types
-      }
 
 
       // Assuming that OldState and NewState have the same number of components

_{Drops the ngrow_loc=0 special cases so every cell-centered state keeps ngrow_loc=1. CellConservativeLinear::CoarseBox (amrex/Src/AmrCore/AMReX_Interpolater.cpp) needs coarsen(fine) grown by 1, so 1 ghost is the minimum workspace; the copy calls use src_nghost=0 and are safe for any source ghost count.}

For Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:672 (F045):

--- a/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
+++ b/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
@@ -669,7 +669,11 @@
 
 
       // Assuming that OldState and NewState have the same number of components
-      int ncomps = (falRef_src.state[n].old_data)->nComp();
+      int ncomps = (falRef_src.state[n].new_data)->nComp();
+
+      // old_data is null when the checkpoint stores only one data set
+      // (nsets == 1), e.g. a checkpoint written right after initialization.
+      bool has_old = (falRef_src.state[n].old_data != nullptr);
 
       BoxArray new_grids_state = falRef_trgt.state[n].grids;
       BoxArray save_grids_state = falRef_trgt.state[n].grids;
@@ -695,7 +699,9 @@
       OldData_src -> setVal(10.);
 
       NewData_src -> copy(*(falRef_src.state[n].new_data),0,0,ncomps,0,ngrow_loc);
-      OldData_src -> copy(*(falRef_src.state[n].old_data),0,0,ncomps,0,ngrow_loc);
+      if (has_old) {
+        OldData_src -> copy(*(falRef_src.state[n].old_data),0,0,ncomps,0,ngrow_loc);
+      }
 
       MultiFab * NewData_trgt = new MultiFab(new_grids_state,dm_trgt,ncomps,ngrow_loc);
       MultiFab * OldData_trgt = new MultiFab(new_grids_state,dm_trgt,ncomps,ngrow_loc);
@@ -750,7 +756,9 @@
       }
 
       falRef_trgt.state[n].new_data = NewData_trgt;
-      falRef_trgt.state[n].old_data = OldData_trgt;
+      // Leave old_data null when the source had none so that the writer
+      // emits a single data set (nsets == 1) for this state.
+      falRef_trgt.state[n].old_data = has_old ? OldData_trgt : nullptr;
 
     }
   }

_{Takes ncomps from new_data and dereferences old_data only when non-null; target old_data stays null so WriteCheckpointFile's dump_old logic emits nsets==1 (consistent with nsets_save). Sentinel-filled OldData_* MultiFabs are still allocated as workspace, keeping the interp/average_down loops untouched — wasted work in the nsets==1 case, but the tool already leaks all these MultiFabs.}

For Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp:697 (F046):

--- a/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
+++ b/Util/ConvertCheckpoint/ConvertCheckpointGrids.cpp
@@ -712,6 +712,12 @@
 
         const Geometry& fgeom = falRef_trgt.geom;
         const Geometry& cgeom = falRef_src.geom;
+
+        // The copies above are non-periodic, so ghost cells at periodic
+        // domain boundaries still hold the setVal(10.) sentinel; fill them
+        // from valid data before computing the interpolation slopes.
+        NewData_src->FillBoundary(cgeom.periodicity());
+        OldData_src->FillBoundary(cgeom.periodicity());
 
         for (MFIter mfi(*NewData_trgt); mfi.isValid(); ++mfi)
         {

_{Mirrors the existing FillBoundary twin on the target MultiFabs, placed in the refine branch (average_down reads no ghosts). Geometry::operator>> restores periodicity from the checkpoint's 'P' marker, so cgeom.periodicity() is correct. Residual: ghosts at NON-periodic physical boundaries still hold 10.0 — unfixable without BC info; maintainer must decide if one-sided slopes there need separate handling.}

_{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

F009 — confirmed (two independent verifier lenses)

Lens 1 (refutation attempt): Line 69 Vector<int> nsets_save(1);, line 282 if (lev == 1) nsets_save.resize(ndesc_save);, line 300 nsets_save[ii] = nsets; for ii=3 always. Lev 0 runs before any resize: OOB write; amrex::Vector asserts only with DEBUG=TRUE. Single-level: writer reads nsets_save[i>=1] OOB (lines 505/517/524). Downgraded: indices 1..5 stay inside glibc's 24-byte min usable chunk, so release is typically symptomless and values round-trip; still UB, ASan/debug abort.

Lens 2 (reachability/intent): Line 69 Vector<int> nsets_save(1); line 300 nsets_save[ii] = nsets runs at lev 0 with ii<ndesc, ndesc>=3 always (NavierStokesBase.H:49 enum State/Press/Gradp); resize at line 282 gated by if (lev == 1). amrex::Vector bounds-checks only under AMREX_DEBUG (AMReX_Vector.H:37); tool GNUmakefile sets DEBUG=FALSE. Pattern copied unchanged from Castro Embiggen.cpp (commit b929df1), never edited — no deliberate-intent evidence. Minor claim error: null old_data write impossible (ConvertData lines 752-753 always allocate), but core OOB write/read stands.

F010 — confirmed (two independent verifier lenses)

Lens 1 (refutation attempt): Lines 660-668 force ngrow_loc=0 for trailing states (sizes 4/5/6); line 723 calls cell_cons_interp (only n==1 gets node_bilinear). AMReX Interpolater.cpp:863-869: cslope_bx = coarsen(fine_region) exactly; MFInterp_C.H:14 dc = 0.5*(u(i+1)-u(i-1)) unconditionally since default BCRec is BCType::bogus. Crse fab box = coarsen(tilebox) with 0 ghosts, so edge slope cells read 1 cell outside the allocation. Reachable with interp_kind=refine on divu/Average checkpoints.

Lens 2 (reachability/intent): ConvertCheckpointGrids.cpp:660-668 force ngrow_loc=0 for trailing states; line 692 allocates cfab with 0 ghosts; lines 723-724 call cell_cons_interp. CellConservativeLinear::interp (AMReX_Interpolater.cpp:864-869) loops slopes over cslope_bx=coarsen(fine bx) and mf_compute_slopes_x (AMReX_MFInterp_C.H:14) unconditionally reads u(i±1) — one cell outside the 0-ghost fab. Commits 9a52128/bdeb4b33 show ngrow_loc mirrors checkpoint ghost counts, not stencil needs; no abort guards the path.

F055 — confirmed (one verifier lens)

Reported as: nsets_save is created with size 1 and only resized when lev==1 is processed, so ReadCheckpointFile writes nsets_save[ii] out of bounds for every descriptor ii>=1 while reading level 0, and single-level checkpoints never resize it at all.

Failure scenario: Convert any IAMR checkpoint (ndesc>=3: State, Press, Gradp): at lev 0 the loop writes 2+ ints past the end of the 1-element heap vector — silent heap corruption in release builds. For a single-level checkpoint, WriteCheckpointFile (line 505) also reads nsets_save[i] out of bounds, potentially emitting wrong nsets and a corrupt converted checkpoint.

Lens 1 (refutation attempt): Duplicate of F009. Line 300 nsets_save[ii] = nsets; writes indices 1..ndesc-1 of the size-1 vector at lev 0 (resize at line 282 gated on lev==1); single-level checkpoints never resize, and WriteCheckpointFile reads nsets_save[i] OOB at lines 505/517/524. Medium is the right call: guaranteed UB and debug/ASan abort, but glibc chunk padding usually masks it in release.

F045 — confirmed (one verifier lens)

Lens 1 (refutation attempt): Line 672 int ncomps = (falRef_src.state[n].old_data)->nComp(); unconditional; reader nulls old_data unless nsets==2 (lines 302/323). IAMR writes nsets==1 whenever old_data is null (AMReX_StateData.cpp:786-818); only Press/Gradp get allocOldData at init (NavierStokesBase.cpp:265-266), so chk00000 with ns.init_iter=0 (post_init_press early-returns, NavierStokes.cpp:1310) plus check_int>0 (Amr.cpp:1183) has State_Type nsets==1 -> null deref. Scenario needs init_iter<=0, not just step 0.

F046 — confirmed (one verifier lens)

Lens 1 (refutation attempt): Lines 697-698 NewData_src->copy(...,0,ngrow_loc) uses default Periodicity::NonPeriodic() (AMReX_FabArray.H:1329-1337); no FillBoundary on the src MultiFabs, so domain-edge ghosts keep setVal(10.) from lines 694-695. Default-constructed BCRec is BCType::bogus, so mf_compute_slopes_* takes the plain central difference through the 10.0 ghost at every domain-edge coarse cell; wrong valid fine cells along all domain edges for ngrow_loc>=1 cell-centered states.

F078 — confirmed (one verifier lens)

Reported as: ConvertData unconditionally dereferences and copies falRef_src.state[n].old_data (lines 672, 698), which ReadCheckpointFile leaves null whenever a state was checkpointed with nsets==1 (new data only).

Failure scenario: Convert a checkpoint whose states lack old data, e.g. chk00000 written right after initialization with ns.init_iter=0 (State_Type has no old data) or with ns.avg_interval>0 (Average_Type new-only): (old_data)->nComp() dereferences a null pointer and the tool segfaults, even though the writer side (lines 484-513) was written to handle nsets<2.

Lens 1 (refutation attempt): Duplicate of F045 with the correct trigger: lines 672/698 unconditionally deref old_data, which ReadCheckpointFile leaves null for nsets==1 (line 302 vs 323). chk00000 with ns.init_iter=0 is reachable (NavierStokes.cpp:1310 early return; Amr.cpp:1183 checkPoint; only Press/Gradp allocOldData at init). The avg_interval sub-scenario is weaker (advance_setup allocOldData's all state types on first advance, NavierStokesBase.cpp:695), but the init_iter=0 case carries the finding.

---

_{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: F009, F010, F055, F045, F046, F078. Reviewer unit(s): Util+Benchmarks, 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.}