From f926fbb0e7c12b3210864acbf0d54997e4a3b5a1 Mon Sep 17 00:00:00 2001 From: Paul Bauer Date: Wed, 5 May 2021 17:16:05 +0200 Subject: [PATCH] Remove UB in ArrayRef from array Several issues with ArrayRefs from nullptr have been found by UBSAN. --- src/gromacs/domdec/mdsetup.cpp | 6 +- src/gromacs/domdec/partition.cpp | 34 ++++---- src/gromacs/listed_forces/listed_forces.cpp | 4 +- src/gromacs/mdlib/force.cpp | 24 ++++-- src/gromacs/mdlib/sim_util.cpp | 77 +++++++++++-------- .../mdlib/tests/leapfrogtestrunners.cpp | 12 ++- 6 files changed, 96 insertions(+), 61 deletions(-) diff --git a/src/gromacs/domdec/mdsetup.cpp b/src/gromacs/domdec/mdsetup.cpp index 53236dd3d8..ead9a61fd1 100644 --- a/src/gromacs/domdec/mdsetup.cpp +++ b/src/gromacs/domdec/mdsetup.cpp @@ -148,8 +148,10 @@ void mdAlgorithmsSetupAtomData(const t_commrec* cr, const int numPmeAtoms = numHomeAtoms - fr->n_tpi; gmx_pme_reinit_atoms(fr->pmedata, numPmeAtoms, - gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr)); + mdatoms->chargeA ? gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->chargeB ? gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr) + : gmx::ArrayRef{}); } if (constr) diff --git a/src/gromacs/domdec/partition.cpp b/src/gromacs/domdec/partition.cpp index 6bd203df67..9a9409422c 100644 --- a/src/gromacs/domdec/partition.cpp +++ b/src/gromacs/domdec/partition.cpp @@ -3285,21 +3285,25 @@ void dd_partition_system(FILE* fplog, if (!thisRankHasDuty(cr, DUTY_PME)) { /* Send the charges and/or c6/sigmas to our PME only node */ - gmx_pme_send_parameters(cr, - *fr->ic, - mdatoms->nChargePerturbed != 0, - mdatoms->nTypePerturbed != 0, - gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr), - mdatoms->chargeB ? gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr) - : gmx::ArrayRef{}, - gmx::arrayRefFromArray(mdatoms->sqrt_c6A, mdatoms->nr), - mdatoms->sqrt_c6B ? gmx::arrayRefFromArray(mdatoms->sqrt_c6B, mdatoms->nr) - : gmx::ArrayRef{}, - gmx::arrayRefFromArray(mdatoms->sigmaA, mdatoms->nr), - mdatoms->sigmaB ? gmx::arrayRefFromArray(mdatoms->sigmaB, mdatoms->nr) - : gmx::ArrayRef{}, - dd_pme_maxshift_x(*dd), - dd_pme_maxshift_y(*dd)); + gmx_pme_send_parameters( + cr, + *fr->ic, + mdatoms->nChargePerturbed != 0, + mdatoms->nTypePerturbed != 0, + mdatoms->chargeA ? gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->chargeB ? gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->sqrt_c6A ? gmx::arrayRefFromArray(mdatoms->sqrt_c6A, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->sqrt_c6B ? gmx::arrayRefFromArray(mdatoms->sqrt_c6B, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->sigmaA ? gmx::arrayRefFromArray(mdatoms->sigmaA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->sigmaB ? gmx::arrayRefFromArray(mdatoms->sigmaB, mdatoms->nr) + : gmx::ArrayRef{}, + dd_pme_maxshift_x(*dd), + dd_pme_maxshift_y(*dd)); } if (dd->atomSets != nullptr) diff --git a/src/gromacs/listed_forces/listed_forces.cpp b/src/gromacs/listed_forces/listed_forces.cpp index be7ba91f0f..85967f1b7f 100644 --- a/src/gromacs/listed_forces/listed_forces.cpp +++ b/src/gromacs/listed_forces/listed_forces.cpp @@ -512,8 +512,8 @@ real calc_one_bond(int thread, pbc, lambda.data(), dvdl.data(), - gmx::arrayRefFromArray(md->chargeA, md->nr), - gmx::arrayRefFromArray(md->chargeB, md->nr), + md->chargeA ? gmx::arrayRefFromArray(md->chargeA, md->nr) : gmx::ArrayRef{}, + md->chargeB ? gmx::arrayRefFromArray(md->chargeB, md->nr) : gmx::ArrayRef{}, md->bPerturbed ? gmx::arrayRefFromArray(md->bPerturbed, md->nr) : gmx::ArrayRef(), gmx::arrayRefFromArray(md->cENER, md->nr), md->nPerturbed, diff --git a/src/gromacs/mdlib/force.cpp b/src/gromacs/mdlib/force.cpp index bfb1ca7b1d..a939841283 100644 --- a/src/gromacs/mdlib/force.cpp +++ b/src/gromacs/mdlib/force.cpp @@ -167,8 +167,10 @@ void calculateLongRangeNonbondeds(t_forcerec* fr, t, *fr, ir, - gmx::constArrayRefFromArray(md->chargeA, md->nr), - gmx::constArrayRefFromArray(md->chargeB, md->nr), + md->chargeA ? gmx::constArrayRefFromArray(md->chargeA, md->nr) + : gmx::ArrayRef{}, + md->chargeB ? gmx::constArrayRefFromArray(md->chargeB, md->nr) + : gmx::ArrayRef{}, (md->nChargePerturbed != 0), coordinates, box, @@ -217,12 +219,18 @@ void calculateLongRangeNonbondeds(t_forcerec* fr, fr->pmedata, gmx::constArrayRefFromArray(coordinates.data(), md->homenr - fr->n_tpi), forceWithVirial->force_, - gmx::constArrayRefFromArray(md->chargeA, md->nr), - gmx::constArrayRefFromArray(md->chargeB, md->nr), - gmx::constArrayRefFromArray(md->sqrt_c6A, md->nr), - gmx::constArrayRefFromArray(md->sqrt_c6B, md->nr), - gmx::constArrayRefFromArray(md->sigmaA, md->nr), - gmx::constArrayRefFromArray(md->sigmaB, md->nr), + md->chargeA ? gmx::constArrayRefFromArray(md->chargeA, md->nr) + : gmx::ArrayRef{}, + md->chargeB ? gmx::constArrayRefFromArray(md->chargeB, md->nr) + : gmx::ArrayRef{}, + md->sqrt_c6A ? gmx::constArrayRefFromArray(md->sqrt_c6A, md->nr) + : gmx::ArrayRef{}, + md->sqrt_c6B ? gmx::constArrayRefFromArray(md->sqrt_c6B, md->nr) + : gmx::ArrayRef{}, + md->sigmaA ? gmx::constArrayRefFromArray(md->sigmaA, md->nr) + : gmx::ArrayRef{}, + md->sigmaB ? gmx::constArrayRefFromArray(md->sigmaB, md->nr) + : gmx::ArrayRef{}, box, cr, DOMAINDECOMP(cr) ? dd_pme_maxshift_x(*cr->dd) : 0, diff --git a/src/gromacs/mdlib/sim_util.cpp b/src/gromacs/mdlib/sim_util.cpp index 342c08bb62..2f82ddc6d3 100644 --- a/src/gromacs/mdlib/sim_util.cpp +++ b/src/gromacs/mdlib/sim_util.cpp @@ -1670,8 +1670,10 @@ void do_force(FILE* fplog, calc_mu(start, mdatoms->homenr, xRef, - gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr), + mdatoms->chargeA ? gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->chargeB ? gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr) + : gmx::ArrayRef{}, mdatoms->nChargePerturbed != 0, dipoleData.muStaging[0], dipoleData.muStaging[1]); @@ -1763,35 +1765,45 @@ void do_force(FILE* fplog, /* Calculate the local and non-local free energy interactions here. * Happens here on the CPU both with and without GPU. */ - nbv->dispatchFreeEnergyKernel(InteractionLocality::Local, - *fr, - x.unpaddedArrayRef(), - &forceOutNonbonded->forceWithShiftForces(), - gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->typeA, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->typeB, mdatoms->nr), - inputrec.fepvals.get(), - lambda, - enerd, - stepWork, - nrnb); + nbv->dispatchFreeEnergyKernel( + InteractionLocality::Local, + *fr, + x.unpaddedArrayRef(), + &forceOutNonbonded->forceWithShiftForces(), + mdatoms->chargeA ? gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->chargeB ? gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->typeA ? gmx::arrayRefFromArray(mdatoms->typeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->typeB ? gmx::arrayRefFromArray(mdatoms->typeB, mdatoms->nr) + : gmx::ArrayRef{}, + inputrec.fepvals.get(), + lambda, + enerd, + stepWork, + nrnb); if (havePPDomainDecomposition(cr)) { - nbv->dispatchFreeEnergyKernel(InteractionLocality::NonLocal, - *fr, - x.unpaddedArrayRef(), - &forceOutNonbonded->forceWithShiftForces(), - gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->typeA, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->typeB, mdatoms->nr), - inputrec.fepvals.get(), - lambda, - enerd, - stepWork, - nrnb); + nbv->dispatchFreeEnergyKernel( + InteractionLocality::NonLocal, + *fr, + x.unpaddedArrayRef(), + &forceOutNonbonded->forceWithShiftForces(), + mdatoms->chargeA ? gmx::arrayRefFromArray(mdatoms->chargeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->chargeB ? gmx::arrayRefFromArray(mdatoms->chargeB, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->typeA ? gmx::arrayRefFromArray(mdatoms->typeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->typeB ? gmx::arrayRefFromArray(mdatoms->typeB, mdatoms->nr) + : gmx::ArrayRef{}, + inputrec.fepvals.get(), + lambda, + enerd, + stepWork, + nrnb); } } @@ -1841,9 +1853,12 @@ void do_force(FILE* fplog, real dvdl_walls = do_walls(inputrec, *fr, box, - gmx::arrayRefFromArray(mdatoms->typeA, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->typeB, mdatoms->nr), - gmx::arrayRefFromArray(mdatoms->cENER, mdatoms->nr), + mdatoms->typeA ? gmx::arrayRefFromArray(mdatoms->typeA, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->typeB ? gmx::arrayRefFromArray(mdatoms->typeB, mdatoms->nr) + : gmx::ArrayRef{}, + mdatoms->cENER ? gmx::arrayRefFromArray(mdatoms->cENER, mdatoms->nr) + : gmx::ArrayRef{}, mdatoms->homenr, mdatoms->nPerturbed, x.unpaddedConstArrayRef(), diff --git a/src/gromacs/mdlib/tests/leapfrogtestrunners.cpp b/src/gromacs/mdlib/tests/leapfrogtestrunners.cpp index 5badaad272..a82d9fc9e7 100644 --- a/src/gromacs/mdlib/tests/leapfrogtestrunners.cpp +++ b/src/gromacs/mdlib/tests/leapfrogtestrunners.cpp @@ -76,9 +76,15 @@ void LeapFrogHostTestRunner::integrate(LeapFrogTestData* testData, int numSteps) step, testData->mdAtoms_.homenr, testData->mdAtoms_.havePartiallyFrozenAtoms, - gmx::arrayRefFromArray(testData->mdAtoms_.ptype, testData->mdAtoms_.nr), - gmx::arrayRefFromArray(testData->mdAtoms_.invmass, testData->mdAtoms_.nr), - gmx::arrayRefFromArray(testData->mdAtoms_.invMassPerDim, testData->mdAtoms_.nr), + testData->mdAtoms_.ptype + ? gmx::arrayRefFromArray(testData->mdAtoms_.ptype, testData->mdAtoms_.nr) + : gmx::ArrayRef{}, + testData->mdAtoms_.invmass + ? gmx::arrayRefFromArray(testData->mdAtoms_.invmass, testData->mdAtoms_.nr) + : gmx::ArrayRef{}, + testData->mdAtoms_.invMassPerDim ? gmx::arrayRefFromArray(testData->mdAtoms_.invMassPerDim, + testData->mdAtoms_.nr) + : gmx::ArrayRef{}, &testData->state_, testData->f_, testData->forceCalculationData_, -- 2.22.0