segfault in ID_converter_ILGRMHD when Avec and Aphi are not given storage outside of ILGRMHD

Create issue
Issue #2394 resolved
Roland Haas created an issue

git hash e46f4f5 "Major update to IllinoisGRMHD/Convert_to_HydroBase/ID_converter_ILGRMHD: Improve efficiency (e.g., remove unnecessary additional Newton-Raphson iterations in con2prim), remove u0 gridfunction, bugfix, separate IllinoisGRMHD::grmhd_primitives_Bi and IllinoisGRMHD::grmhd_primitives_Bi_stagger" of wvuthorns introduced these lines:

  if(pure_hydro_run) {
#pragma omp parallel for
    for(int k=0;k<cctk_lsh[2];k++) for(int j=0;j<cctk_lsh[1];j++) for(int i=0;i<cctk_lsh[0];i++) {
      int index=CCTK_GFINDEX3D(cctkGH,i,j,k);

which cause asegfault when AVec or Aphi have no storage. ID_converter_ILGRMHD does not take measures to ensure that there actually be storage so eg the example parfile from segfaults.

ID_converter_ILGRMHD (and presumably) ILGRMHD itself must either ensure that there is storage for variables they unconditionally access or contain logic to avoid accessing variables that have no storage.

Comments (7)

  1. Zach Etienne

    This can be fixed with just a few-line patch, right?

    Here’s a proposed patch:

    diff --git a/ID_converter_ILGRMHD/schedule.ccl b/ID_converter_ILGRMHD/schedule.ccl
    index 626c9a6..57f5c1a 100644
    --- a/ID_converter_ILGRMHD/schedule.ccl
    +++ b/ID_converter_ILGRMHD/schedule.ccl
    @@ -1,5 +1,9 @@
     # Schedule definitions for thorn ID_converter_ILGRMHD
    +if (!pure_hydro_run) {
    +   STORAGE: HydroBase::Avec[1] HydroBase::Aphi[1]
     schedule group IllinoisGRMHD_ID_Converter at CCTK_INITIAL after HydroBase_Initial before Convert_to_HydroBase
     } "Translate ET-generated, HydroBase-compatible initial data and convert into variables used by IllinoisGRMHD"

  2. Roland Haas reporter

    That may fix it, yes. Are there any other variables that are similarly used and do not always have storage? ADMBase variables and maybe Bvec in HydroBase?

  3. Zach Etienne

    A single timelevel of storage is allocated for Bvec in Convert_to_HydroBase & GiRaFFE_to_HydroBase, which are the only thorns that write to it.

    Regarding ADMBase variables, they must first be set in CCTK_INITIAL outside IllinoisGRMHD, otherwise it doesn’t make sense to be using ID_converter_* (which by its nature should be able to assume that ID already exist!) The story is the same for vel[], rho, press, and eps gridfunctions – these must be set within CCTK_INITIAL or it doesn’t make sense to be calling on an ID converter thorn.

    There was a similar issue in ID_converter_GiRaFFE, in which there was the same parameter (lazily copied from ILGRMHD) “pure_hydro_run”, but this is nonsense in the case of GRFFE (which assumes a magnetically dominated plasma), so I removed it.

    First attempt at a fix:

    Second time’s a charm:

  4. Roland Haas reporter

    I can confirm that git hash e1e4d3c "WVUThorns: ID_converter_ILGRMHD -- allocate storage for Avec and Aphi regardless of how pure_hydro_run parameter is set." of wvuthorns avoids the SEGFAULT in the example parfile tov_star_parfile_for_IllinoisGRMHD.par

  5. Roland Haas reporter

    I would normally suggest this to be backported to the release branch (as an important fix). However given that the next release is in 11 days this may not be needed.

  6. Log in to comment