From d63e5c42affd65b950f57046c3274e649ce3f127 Mon Sep 17 00:00:00 2001 From: Teemu Murtola Date: Fri, 27 Jun 2014 14:20:39 +0300 Subject: [PATCH] Additional index group checks for selections Now index groups provided to selections are checked more thoroughly, and an error is given if a group references an atom index outside the topology (or the maximum atom number in the absence of a topology). Negative atom indices also trigger the same error. Previously, such index groups were silently accepted (except for a warning during reading for negative indices), and the out-of-range atoms dropped from the selections. Change-Id: Id7a024ed316920d9594454ae36f7b9bdd9a429fc --- src/gromacs/selection/indexutil.cpp | 13 +++++ src/gromacs/selection/indexutil.h | 11 ++++ src/gromacs/selection/parsetree.cpp | 10 ++-- src/gromacs/selection/selectioncollection.cpp | 54 ++++++++++++++++++- src/gromacs/selection/selelem.cpp | 23 +++++++- src/gromacs/selection/selelem.h | 16 ++++-- .../selection/tests/selectioncollection.cpp | 21 ++++++++ .../trajectoryanalysis/runnercommon.cpp | 11 ---- 8 files changed, 139 insertions(+), 20 deletions(-) diff --git a/src/gromacs/selection/indexutil.cpp b/src/gromacs/selection/indexutil.cpp index f921caeaac..3ed8c8783e 100644 --- a/src/gromacs/selection/indexutil.cpp +++ b/src/gromacs/selection/indexutil.cpp @@ -440,6 +440,19 @@ gmx_ana_index_check_sorted(gmx_ana_index_t *g) return true; } +bool +gmx_ana_index_check_range(gmx_ana_index_t *g, int natoms) +{ + for (int i = 0; i < g->isize; ++i) + { + if (g->index[i] < 0 || g->index[i] >= natoms) + { + return false; + } + } + return true; +} + /******************************************************************** * Set operations ********************************************************************/ diff --git a/src/gromacs/selection/indexutil.h b/src/gromacs/selection/indexutil.h index f9b4b3f46c..4575879009 100644 --- a/src/gromacs/selection/indexutil.h +++ b/src/gromacs/selection/indexutil.h @@ -239,6 +239,17 @@ gmx_ana_index_dump(FILE *fp, gmx_ana_index_t *g, int maxn); /** Checks whether an index group is sorted. */ bool gmx_ana_index_check_sorted(gmx_ana_index_t *g); + +/*! \brief + * Checks whether an index group has atoms from a defined range. + * + * \param[in] g Index group to check. + * \param[in] natoms Largest atom number allowed. + * \returns true if all atoms in the index group are in the + * range 0 to \p natoms (i.e., no atoms over \p natoms are referenced). + */ +bool +gmx_ana_index_check_range(gmx_ana_index_t *g, int natoms); /*@}*/ /*! \name Functions for set operations on gmx_ana_index_t diff --git a/src/gromacs/selection/parsetree.cpp b/src/gromacs/selection/parsetree.cpp index 14f55a9a6a..3ba3e572a9 100644 --- a/src/gromacs/selection/parsetree.cpp +++ b/src/gromacs/selection/parsetree.cpp @@ -915,8 +915,9 @@ _gmx_sel_init_group_by_name(const char *name, yyscan_t scanner) if (_gmx_sel_lexer_has_groups_set(scanner)) { - gmx_ana_indexgrps_t *grps = _gmx_sel_lexer_indexgrps(scanner); - sel->resolveIndexGroupReference(grps); + gmx_ana_indexgrps_t *grps = _gmx_sel_lexer_indexgrps(scanner); + gmx_ana_selcollection_t *sc = _gmx_sel_lexer_selcollection(scanner); + sel->resolveIndexGroupReference(grps, sc->gall.isize); } return sel; @@ -938,8 +939,9 @@ _gmx_sel_init_group_by_id(int id, yyscan_t scanner) if (_gmx_sel_lexer_has_groups_set(scanner)) { - gmx_ana_indexgrps_t *grps = _gmx_sel_lexer_indexgrps(scanner); - sel->resolveIndexGroupReference(grps); + gmx_ana_indexgrps_t *grps = _gmx_sel_lexer_indexgrps(scanner); + gmx_ana_selcollection_t *sc = _gmx_sel_lexer_selcollection(scanner); + sel->resolveIndexGroupReference(grps, sc->gall.isize); } return sel; diff --git a/src/gromacs/selection/selectioncollection.cpp b/src/gromacs/selection/selectioncollection.cpp index 8500bc1518..2d766bcdf4 100644 --- a/src/gromacs/selection/selectioncollection.cpp +++ b/src/gromacs/selection/selectioncollection.cpp @@ -423,6 +423,44 @@ early_termination: return result; } +/*! \brief + * Checks that index groups have valid atom indices. + * + * \param[in] root Root of selection tree to process. + * \param[in] natoms Maximum number of atoms that the selections are set + * to evaluate. + * \param errors Object for reporting any error messages. + * \throws std::bad_alloc if out of memory. + * + * Recursively checks the selection tree for index groups. + * Each found group is checked that it only contains atom indices that match + * the topology/maximum number of atoms set for the selection collection. + * Any issues are reported to \p errors. + */ +void checkExternalGroups(const SelectionTreeElementPointer &root, + int natoms, + ExceptionInitializer *errors) +{ + if (root->type == SEL_CONST && root->v.type == GROUP_VALUE) + { + try + { + root->checkIndexGroup(natoms); + } + catch (const UserInputError &) + { + errors->addCurrentExceptionAsNested(); + } + } + + SelectionTreeElementPointer child = root->child; + while (child) + { + checkExternalGroups(child, natoms, errors); + child = child->next; + } +} + } // namespace @@ -435,7 +473,7 @@ void SelectionCollection::Impl::resolveExternalGroups( { try { - root->resolveIndexGroupReference(grps_); + root->resolveIndexGroupReference(grps_, sc_.gall.isize); } catch (const UserInputError &) { @@ -552,6 +590,20 @@ SelectionCollection::setTopology(t_topology *top, int natoms) { natoms = top->atoms.nr; } + if (impl_->bExternalGroupsSet_) + { + ExceptionInitializer errors("Invalid index group references encountered"); + SelectionTreeElementPointer root = impl_->sc_.root; + while (root) + { + checkExternalGroups(root, natoms, &errors); + root = root->next; + } + if (errors.hasNestedExceptions()) + { + GMX_THROW(InconsistentInputError(errors)); + } + } gmx_ana_selcollection_t *sc = &impl_->sc_; // Do this first, as it allocates memory, while the others don't throw. gmx_ana_index_init_simple(&sc->gall, natoms); diff --git a/src/gromacs/selection/selelem.cpp b/src/gromacs/selection/selelem.cpp index a10d5a5802..b396fa1a11 100644 --- a/src/gromacs/selection/selelem.cpp +++ b/src/gromacs/selection/selelem.cpp @@ -366,7 +366,8 @@ void SelectionTreeElement::checkUnsortedAtoms( } } -void SelectionTreeElement::resolveIndexGroupReference(gmx_ana_indexgrps_t *grps) +void SelectionTreeElement::resolveIndexGroupReference( + gmx_ana_indexgrps_t *grps, int natoms) { GMX_RELEASE_ASSERT(type == SEL_GROUPREF, "Should only be called for index group reference elements"); @@ -411,6 +412,26 @@ void SelectionTreeElement::resolveIndexGroupReference(gmx_ana_indexgrps_t *grps) gmx_ana_index_set(&u.cgrp, foundGroup.isize, foundGroup.index, foundGroup.nalloc_index); setName(foundName); + + if (natoms > 0) + { + checkIndexGroup(natoms); + } +} + +void SelectionTreeElement::checkIndexGroup(int natoms) +{ + GMX_RELEASE_ASSERT(type == SEL_CONST && v.type == GROUP_VALUE, + "Should only be called for index group elements"); + if (!gmx_ana_index_check_range(&u.cgrp, natoms)) + { + std::string message = formatString( + "Group '%s' cannot be used in selections, because it " + "contains negative atom indices and/or references atoms " + "not present (largest allowed atom index is %d).", + name().c_str(), natoms); + GMX_THROW(InconsistentInputError(message)); + } } } // namespace gmx diff --git a/src/gromacs/selection/selelem.h b/src/gromacs/selection/selelem.h index 13c0e949f8..1a1e86bf61 100644 --- a/src/gromacs/selection/selelem.h +++ b/src/gromacs/selection/selelem.h @@ -339,14 +339,24 @@ class SelectionTreeElement void checkUnsortedAtoms(bool bUnsortedAllowed, ExceptionInitializer *errors) const; /*! \brief - * Resolved an unresolved reference to an index group. + * Resolves an unresolved reference to an index group. * - * \param[in] grps Index groups to use to resolve the reference. + * \param[in] grps Index groups to use to resolve the reference. + * \param[in] natoms Maximum number of atoms the selections can evaluate to + * (zero if the topology/atom count is not set yet). * \throws std::bad_alloc if out of memory. * \throws InconsistentInputError if the reference cannot be * resolved. */ - void resolveIndexGroupReference(gmx_ana_indexgrps_t *grps); + void resolveIndexGroupReference(gmx_ana_indexgrps_t *grps, int natoms); + /*! \brief + * Checks that an index group has valid atom indices. + * + * \param[in] natoms Maximum number of atoms the selections can evaluate to. + * \throws std::bad_alloc if out of memory. + * \throws InconsistentInputError if there are invalid atom indices. + */ + void checkIndexGroup(int natoms); //! Type of the element. e_selelem_t type; diff --git a/src/gromacs/selection/tests/selectioncollection.cpp b/src/gromacs/selection/tests/selectioncollection.cpp index 8cfcae13c6..7a22779ef2 100644 --- a/src/gromacs/selection/tests/selectioncollection.cpp +++ b/src/gromacs/selection/tests/selectioncollection.cpp @@ -510,6 +510,27 @@ TEST_F(SelectionCollectionTest, HandlesUnsortedGroupReferenceDelayed) // EXPECT_THROW_GMX(sc_.compile(), gmx::APIError); } +TEST_F(SelectionCollectionTest, HandlesOutOfRangeAtomIndexInGroup) +{ + ASSERT_NO_THROW_GMX(sc_.setTopology(NULL, 5)); + ASSERT_NO_THROW_GMX(loadIndexGroups("simple.ndx")); + EXPECT_THROW_GMX(sc_.parseFromString("group \"GrpB\""), gmx::InconsistentInputError); +} + +TEST_F(SelectionCollectionTest, HandlesOutOfRangeAtomIndexInGroupDelayed) +{ + ASSERT_NO_THROW_GMX(loadIndexGroups("simple.ndx")); + ASSERT_NO_THROW_GMX(sc_.parseFromString("group \"GrpB\"")); + EXPECT_THROW_GMX(sc_.setTopology(NULL, 5), gmx::InconsistentInputError); +} + +TEST_F(SelectionCollectionTest, HandlesOutOfRangeAtomIndexInGroupDelayed2) +{ + ASSERT_NO_THROW_GMX(sc_.setTopology(NULL, 5)); + ASSERT_NO_THROW_GMX(sc_.parseFromString("group \"GrpB\"")); + EXPECT_THROW_GMX(loadIndexGroups("simple.ndx"), gmx::InconsistentInputError); +} + TEST_F(SelectionCollectionTest, RecoversFromMissingMoleculeInfo) { ASSERT_NO_THROW_GMX(sc_.parseFromString("molindex 1 to 5")); diff --git a/src/gromacs/trajectoryanalysis/runnercommon.cpp b/src/gromacs/trajectoryanalysis/runnercommon.cpp index 70a0efd2a4..d61ab99a32 100644 --- a/src/gromacs/trajectoryanalysis/runnercommon.cpp +++ b/src/gromacs/trajectoryanalysis/runnercommon.cpp @@ -373,17 +373,6 @@ TrajectoryAnalysisRunnerCommon::initFirstFrame() "Trajectory (%d atoms) does not match topology (%d atoms)", impl_->fr->natoms, top.topology()->atoms.nr))); } - // TODO: Check index groups if they have been initialized based on the topology. - /* - if (top) - { - for (int i = 0; i < impl_->sel->nr(); ++i) - { - gmx_ana_index_check(impl_->sel->sel(i)->indexGroup(), - impl_->fr->natoms); - } - } - */ } else { -- 2.22.0