z-lo direction_dependent MAC BC reads cc_arr(1,j,k-1,2) instead of (i,j,k-1,2) — OOB read

[WeiqunZhang]

Severity: high/medium · Category: correctness · Fix order: 1 of 29 — fix this first.

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

Location: src/convection/incflo_compute_MAC_projected_velocities.cpp:254

Based on commit 7307d872 (line numbers refer to that tree).

All three findings are the same single-character typo on line 254 of src/convection/incflo_compute_MAC_projected_velocities.cpp; one fix resolves them all.

The defect

src/convection/incflo_compute_MAC_projected_velocities.cpp:254 — In the z-lo direction_dependent BC check, cc_arr(1,j,k-1,2) uses literal 1 instead of loop index i, so the inflow decision reads the wrong cell and can read out of bounds; the analogous x/y blocks (lines 225, 239) correctly use i/j.

src/convection/incflo_compute_MAC_projected_velocities.cpp:254 — In the z-lo direction_dependent inflow test, the w-velocity is sampled at cc_arr(1,j,k-1,2) with literal x-index 1 instead of i, unlike the x/y blocks and the assignment on the next line.

src/convection/incflo_compute_MAC_projected_velocities.cpp:254 — Copy-paste index typo in the z-lo direction-dependent BC test: cc_arr(1,j,k-1,2) uses literal x-index 1 instead of i, diverging from the x-block (i-1,j,k,0), y-block (i,j-1,k,1), and its own z-hi twin (i,j,k+1,2). (Found while verifying my unit's callers; outside assigned file list.)

Why it matters

F002: 3D run with zlo.type = direction_dependent (or dd): for any grid box whose i-range excludes 1, the Array4 read is out of bounds (garbage or ASAN/bounds-check crash); even in-bounds, whether wmac at the zlo face is overridden with the boundary value is decided by w at i=1 instead of at i, giving wrong inflow faces fed to the MAC projection.

F011: 3D run with zlo.type=direction_dependent: for any box whose x-range excludes i=1 the Array4 read is out of the FAB's bounds (garbage/segfault); even in-bounds, wmac(i,j,dlo.z) inflow/outflow selection uses another column's velocity, giving wrong MAC inflow every predictor/corrector step.

F041: 3D run with BC::direction_dependent on the z-lo domain face and a spatially varying boundary w-velocity: the inflow/outflow decision for every (i,j) face is made from the ghost w at the fixed x=1 column, so w_mac on the z-lo boundary is forced to (or left off) the Dirichlet inflow value at the wrong faces, corrupting the MAC BC and divergence constraint near that boundary.

How to reach it

• 3D run based on test_no_eb_3d/benchmark.inout with zlo.type = "direction_dependent" (zlo.velocity set), n_cell 64 32 32, max_grid_size_x 16: boxes with i-range excluding 1 make the read out of bounds.
• 3D run: take test_no_eb_3d/benchmark.inout, set zlo.type=zhi.type="direction_dependent", xlo/xhi="sw"; add amr.max_grid_size=32 with n_cell=64 so a zlo box excludes x-index 1, triggering the OOB read.

Suggested fix

Replace the literal 1 with the loop index i in the z-lo inflow test on line 254. The direction_dependent semantics (AMReX-Hydro BCType::direction_dependent) is that a domain face is treated as inflow only when the ghost-cell velocity component at that face points into the domain, so the sign test must sample exactly the ghost cell whose value would be imposed — the same cc_arr(i,j,k-1,2) that line 255 assigns to wmac_arr(i,j,k). Every twin block already does this: x-lo tests cc_arr(i-1,j,k,0) (line 225), y-lo tests cc_arr(i,j-1,k,1) (line 239), and the z-hi branch tests cc_arr(i,j,k+1,2) (line 258). The fix also removes the out-of-bounds Array4 read on boxes whose grown x-range excludes 1, since time_dep_inflow_vel carries only one ghost cell per FAB. Maintainer call: consider a multi-box-in-x regression test with zlo.type = direction_dependent (or a CI run with AMREX_USE_ASSERTION/bounds checking), and check whether this block was copied into downstream codes after 60881d3.

For src/convection/incflo_compute_MAC_projected_velocities.cpp:254 (F002):

--- a/src/convection/incflo_compute_MAC_projected_velocities.cpp
+++ b/src/convection/incflo_compute_MAC_projected_velocities.cpp
@@ -251,7 +251,7 @@
                 int n = 2;
                 const auto bc = HydroBC::getBC(i, j, k, n, domain, bc_vel_d, velbc_arr);
                 if (k == dlo.z && ( bc.lo(2) == BCType::ext_dir ||
-                                   (bc.lo(2) == BCType::direction_dependent && cc_arr(1,j,k-1,2) >= Real(0.0)) ) ) {
+                                   (bc.lo(2) == BCType::direction_dependent && cc_arr(i,j,k-1,2) >= Real(0.0)) ) ) {
                     wmac_arr(i,j,k) = cc_arr(i,j,k-1,2);
                 }
                 if (k == dhi.z && ( bc.hi(2) == BCType::ext_dir ||

_{Replaces the literal 1 with loop index i so the z-lo direction_dependent inflow test reads cc_arr(i,j,k-1,2) — the same ghost cell assigned on the next line — matching the x-lo (i-1), y-lo (j-1), and z-hi (k+1) twins. Also removes the out-of-bounds read on boxes whose i-range excludes 1.}

_{Diff(s) are against 7307d872, 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

F002 — confirmed (two independent verifier lenses)

Lens 1 (refutation attempt): Line 254: bc.lo(2) == BCType::direction_dependent && cc_arr(1,j,k-1,2) >= Real(0.0) while line 255 assigns wmac_arr(i,j,k) = cc_arr(i,j,k-1,2). Twins use loop indices (line 225: cc_arr(i-1,j,k,0); line 239: cc_arr(i,j-1,k,1); line 258: cc_arr(i,j,k+1,2)). time_dep_inflow_vel has only 1 ghost cell and MFIter iterates per-FAB, so boxes whose grown x-range excludes 1 read outside the FAB. git log -S shows the typo introduced in 60881d3, never fixed.

Lens 2 (reachability/intent): src/convection/incflo_compute_MAC_projected_velocities.cpp:254 reads cc_arr(1,j,k-1,2) in the zlo direction_dependent test while the write at 255 and the x/y analogues (225, 239) use loop indices; introduced uncommented in 60881d3 "enable direction-dependent bcs (#122)". Parser (boundary_conditions.cpp:88-112) accepts zlo.type="dd" with no abort; the loop (197-264) runs unconditionally, and the result feeds enforceInOutSolvability and macproj->setUMAC unmasked.

F011 — confirmed (two independent verifier lenses)

Lens 1 (refutation attempt): Same defect: line 254 cc_arr(1,j,k-1,2) vs line 255's cc_arr(i,j,k-1,2). Reachable via zlo.type = direction_dependent (boundary_conditions.cpp:88-95 sets BCType::direction_dependent on all velocity bcrecs); executed every predictor/corrector step. Per-FAB Array4 over validbox grown by 1 makes the read OOB when the box's x-range excludes i=1; otherwise the inflow decision for column i uses column 1's ghost w.

Lens 2 (reachability/intent): Line 254 still reads cc_arr(1,j,k-1,2); sibling gates (lines 225/229/239/243/258) and the assignment on line 255 all use loop indices. Introduced by commit 60881d3 "enable direction-dependent bcs (#122)" with no comment; clearly a typo, not intent. BC parser (boundary_conditions.cpp:88-113) accepts zlo.type=direction_dependent with no abort; the loop (lines 197-264) runs unconditionally each MAC step. cc_arr spans only the FAB+1 ghost, so boxes excluding i=1 read out of bounds; wmac values feed enforceInOutSolvability and the MAC projection unmasked.

F041 — confirmed (one verifier lens)

Lens 1 (refutation attempt): Same copy-paste typo confirmed at line 254: cc_arr(1,j,k-1,2) diverges from x-block (i-1,j,k,0), y-block (i,j-1,k,1), z-hi twin (i,j,k+1,2), and its own assignment (i,j,k-1,2). With spatially varying ghost w, every (i,j) z-lo face decision uses the x=1 column, corrupting w_mac BCs fed to the MAC projection; multi-box-in-x runs additionally read out of the FAB's bounds, so severity is high, not medium.

---

_{Based on commit 7307d872, which is also the tree the audit verified against. From an automated audit of src/. Audit finding ids: F002, F011, F041. Reviewer unit(s): Convection, Setup+Utilities, theme:eb-smallcell, theme:ghost-fillpatch, theme:gpu-capture, theme:ntrac-ncomp, theme:symmetry-twins. 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.}