From 8ba1ca9eec0aa1f51bd75c5e78c0b870c1eb6329 Mon Sep 17 00:00:00 2001 From: Martin Kronbichler Date: Sat, 23 Apr 2016 20:28:48 +0200 Subject: [PATCH] Fix memory leak in Trilinos vector --- doc/news/changes.h | 9 ++ include/deal.II/lac/trilinos_vector.h | 42 +++++- source/lac/trilinos_vector.cc | 131 ++++++++---------- tests/lac/vector_reinit_01.cc | 107 ++++++++++++++ tests/lac/vector_reinit_01.output | 4 + tests/lac/vector_reinit_02.cc | 108 +++++++++++++++ ...vector_reinit_02.with_trilinos=true.output | 7 + tests/lac/vector_reinit_03.cc | 108 +++++++++++++++ .../vector_reinit_03.with_petsc=true.output | 4 + 9 files changed, 439 insertions(+), 81 deletions(-) create mode 100644 tests/lac/vector_reinit_01.cc create mode 100644 tests/lac/vector_reinit_01.output create mode 100644 tests/lac/vector_reinit_02.cc create mode 100644 tests/lac/vector_reinit_02.with_trilinos=true.output create mode 100644 tests/lac/vector_reinit_03.cc create mode 100644 tests/lac/vector_reinit_03.with_petsc=true.output diff --git a/doc/news/changes.h b/doc/news/changes.h index 4878966edc..7ba12b45f3 100644 --- a/doc/news/changes.h +++ b/doc/news/changes.h @@ -201,6 +201,15 @@ inconvenience this causes.

Specific improvements

    +
  1. Fixed: TrilinosWrappers::MPI::Vector and TrilinosWrappers::Vector could + access invalid memory in the reinit() method if the MPI communicator was + deleted before termination of the program. This usually happened when using + vectors from GrowingVectorMemory where a pool keeps vector alive. This has + been fixed. +
    + (Martin Kronbichler, 2016/04/23) +
    2. +
  2. Fixed: The methods TrilinosWrappers::SparseMatrix::(T)mmult previously produced invalid matrix sizes if the final matrix was non-square. This has been fixed. diff --git a/include/deal.II/lac/trilinos_vector.h b/include/deal.II/lac/trilinos_vector.h index 2954a48f44..da8b33c621 100644 --- a/include/deal.II/lac/trilinos_vector.h +++ b/include/deal.II/lac/trilinos_vector.h @@ -307,8 +307,12 @@ namespace TrilinosWrappers * dimension and the parallel distribution of the input vector, but does * not copy the elements in v. If omit_zeroing_entries * is not true, the elements in the vector are initialized with - * zero, otherwise the content will be left unchanged and the user has - * to set all elements. + * zero. If it is set to true, the vector entries are in an + * unspecified state and the user has to set all elements. In the + * current implementation, this method does not touch the vector entries + * in case the vector layout is unchanged from before, otherwise entries + * are set to zero. Note that this behavior might change between + * releases without notification. * * This function has a third argument, allow_different_maps, * that allows for an exchange of data between two equal-sized vectors @@ -567,7 +571,11 @@ namespace TrilinosWrappers * Reinit functionality. This function destroys the old vector content * and generates a new one based on the input partitioning. The flag * omit_zeroing_entries determines whether the vector should be - * filled with zero (false) or left untouched (true). + * filled with zero (false). If the flag is set to true, the + * vector entries are in an unspecified state and the user has to set + * all elements. In the current implementation, this method still sets + * the entries to zero, but this might change between releases without + * notification. * * * Depending on whether the @p parallel_partitioning argument uniquely @@ -796,7 +804,12 @@ namespace TrilinosWrappers /** * Reinit function that resizes the vector to the size specified by - * n. + * n. The flag omit_zeroing_entries specifies whether + * the vector entries should be initialized to zero (false). If it is set + * to true, the vector entries are in an unspecified state and + * the user has to set all elements. In the current implementation, this + * method still sets the entries to zero, but this might change between + * releases without notification. */ void reinit (const size_type n, const bool omit_zeroing_entries = false); @@ -808,7 +821,11 @@ namespace TrilinosWrappers * in the localized vector should be imported from a distributed vector * that has been initialized with the same communicator. The variable * omit_zeroing_entries determines whether the vector should be - * filled with zero or left untouched. + * filled with zero (false). If it is set to true, the vector + * entries are in an unspecified state and the user has to set all + * elements. In the current implementation, this method still sets the + * entries to zero, but this might change between releases without + * notification. * * Which element of the @p input_map argument are set is in fact ignored, * the only thing that matters is the size of the index space described by @@ -824,7 +841,11 @@ namespace TrilinosWrappers * in the localized vector should be imported from a distributed vector * that has been initialized with the same communicator. The variable * omit_zeroing_entries determines whether the vector should be - * filled with zero (false) or left untouched (true). + * filled with zero (false). If it is set to true, the vector + * entries are in an unspecified state and the user has to set all + * elements. In the current implementation, this method still sets the + * entries to zero, but this might change between releases without + * notification. * * Which element of the @p input_map argument are set is in fact ignored, * the only thing that matters is the size of the index space described by @@ -837,6 +858,15 @@ namespace TrilinosWrappers /** * Reinit function. Takes the information of a Vector and copies * everything to the calling vector, now also allowing different maps. + * + * If the optional flag omit_zeroing_entries is not + * true, the elements in the vector are initialized with zero. If + * it is set to true, the vector entries are in an unspecified + * state and the user has to set all elements. In the current + * implementation, this method does not touch the vector entries in case + * the vector layout is unchanged from before, otherwise entries are set + * to zero. Note that this behavior might change between releases without + * notification. */ void reinit (const VectorBase &V, const bool omit_zeroing_entries = false, diff --git a/source/lac/trilinos_vector.cc b/source/lac/trilinos_vector.cc index b0cbba0a40..52479dfe80 100644 --- a/source/lac/trilinos_vector.cc +++ b/source/lac/trilinos_vector.cc @@ -199,18 +199,11 @@ namespace TrilinosWrappers void Vector::reinit (const Epetra_Map &input_map, - const bool omit_zeroing_entries) + const bool /*omit_zeroing_entries*/) { nonlocal_vector.reset(); - if (vector->Map().SameAs(input_map)==false) - vector.reset (new Epetra_FEVector(input_map)); - else if (omit_zeroing_entries == false) - { - const int ierr = vector->PutScalar(0.); - (void)ierr; - Assert (ierr == 0, ExcTrilinosError(ierr)); - } + vector.reset (new Epetra_FEVector(input_map)); has_ghosts = vector->Map().UniqueGIDs()==false; last_action = Zero; @@ -244,7 +237,19 @@ namespace TrilinosWrappers // with the map in v, and generate the vector. if (allow_different_maps == false) { - if (vector->Map().SameAs(v.vector->Map()) == false) + // check equality for MPI communicators: We can only choose the fast + // version in case the underlying Epetra_MpiComm object is the same, + // otherwise we might access an MPI_Comm object that has been + // deleted +#ifdef DEAL_II_WITH_MPI + const Epetra_MpiComm *my_comm = dynamic_cast(&vector->Comm()); + const Epetra_MpiComm *v_comm = dynamic_cast(&v.vector->Comm()); + const bool same_communicators = my_comm != NULL && v_comm != NULL && + my_comm->DataPtr() == v_comm->DataPtr(); +#else + const bool same_communicators = true; +#endif + if (!same_communicators || vector->Map().SameAs(v.vector->Map()) == false) { vector.reset (new Epetra_FEVector(v.vector->Map())); has_ghosts = v.has_ghosts; @@ -252,11 +257,8 @@ namespace TrilinosWrappers } else if (omit_zeroing_entries == false) { - // old and new vectors - // have exactly the - // same map, i.e. size - // and parallel - // distribution + // old and new vectors have exactly the same map, i.e. size and + // parallel distribution int ierr; ierr = vector->GlobalAssemble (last_action); (void)ierr; @@ -397,11 +399,22 @@ namespace TrilinosWrappers Vector & Vector::operator = (const Vector &v) { + // check equality for MPI communicators to avoid accessing a possibly + // invalid MPI_Comm object +#ifdef DEAL_II_WITH_MPI + const Epetra_MpiComm *my_comm = dynamic_cast(&vector->Comm()); + const Epetra_MpiComm *v_comm = dynamic_cast(&v.vector->Comm()); + const bool same_communicators = my_comm != NULL && v_comm != NULL && + my_comm->DataPtr() == v_comm->DataPtr(); +#else + const bool same_communicators = true; +#endif + // distinguish three cases. First case: both vectors have the same // layout (just need to copy the local data, not reset the memory and // the underlying Epetra_Map). The third case means that we have to // rebuild the calling vector. - if (vector->Map().SameAs(v.vector->Map())) + if (same_communicators && vector->Map().SameAs(v.vector->Map())) { *vector = *v.vector; if (v.nonlocal_vector.get() != 0) @@ -562,24 +575,11 @@ namespace TrilinosWrappers void Vector::reinit (const size_type n, - const bool omit_zeroing_entries) + const bool /*omit_zeroing_entries*/) { - if (size() != n) - { - Epetra_LocalMap map ((TrilinosWrappers::types::int_type)n, 0, - Utilities::Trilinos::comm_self()); - vector.reset (new Epetra_FEVector (map)); - } - else if (omit_zeroing_entries == false) - { - int ierr; - ierr = vector->GlobalAssemble(last_action); - (void)ierr; - Assert (ierr == 0, ExcTrilinosError(ierr)); - - ierr = vector->PutScalar(0.0); - Assert (ierr == 0, ExcTrilinosError(ierr)); - } + Epetra_LocalMap map ((TrilinosWrappers::types::int_type)n, 0, + Utilities::Trilinos::comm_self()); + vector.reset (new Epetra_FEVector (map)); last_action = Zero; } @@ -588,25 +588,12 @@ namespace TrilinosWrappers void Vector::reinit (const Epetra_Map &input_map, - const bool omit_zeroing_entries) + const bool /*omit_zeroing_entries*/) { - if (n_global_elements(vector->Map()) != n_global_elements(input_map)) - { - Epetra_LocalMap map (n_global_elements(input_map), - input_map.IndexBase(), - input_map.Comm()); - vector.reset (new Epetra_FEVector (map)); - } - else if (omit_zeroing_entries == false) - { - int ierr; - ierr = vector->GlobalAssemble(last_action); - (void)ierr; - Assert (ierr == 0, ExcTrilinosError(ierr)); - - ierr = vector->PutScalar(0.0); - Assert (ierr == 0, ExcTrilinosError(ierr)); - } + Epetra_LocalMap map (n_global_elements(input_map), + input_map.IndexBase(), + input_map.Comm()); + vector.reset (new Epetra_FEVector (map)); last_action = Zero; } @@ -616,31 +603,17 @@ namespace TrilinosWrappers void Vector::reinit (const IndexSet &partitioning, const MPI_Comm &communicator, - const bool omit_zeroing_entries) + const bool /*omit_zeroing_entries*/) { - if (n_global_elements(vector->Map()) != - static_cast(partitioning.size())) - { - Epetra_LocalMap map (static_cast(partitioning.size()), - 0, + Epetra_LocalMap map (static_cast(partitioning.size()), + 0, #ifdef DEAL_II_WITH_MPI - Epetra_MpiComm(communicator)); + Epetra_MpiComm(communicator)); #else - Epetra_SerialComm()); - (void)communicator; + Epetra_SerialComm()); + (void)communicator; #endif - vector.reset (new Epetra_FEVector(map)); - } - else if (omit_zeroing_entries == false) - { - int ierr; - ierr = vector->GlobalAssemble(last_action); - (void)ierr; - Assert (ierr == 0, ExcTrilinosError(ierr)); - - ierr = vector->PutScalar(0.0); - Assert (ierr == 0, ExcTrilinosError(ierr)); - } + vector.reset (new Epetra_FEVector(map)); last_action = Zero; } @@ -659,17 +632,25 @@ namespace TrilinosWrappers // the vector, initialize our // map with the map in v, and // generate the vector. - (void)omit_zeroing_entries; if (allow_different_maps == false) { - if (local_range() != v.local_range()) + // check equality for MPI communicators +#ifdef DEAL_II_WITH_MPI + const Epetra_MpiComm *my_comm = dynamic_cast(&vector->Comm()); + const Epetra_MpiComm *v_comm = dynamic_cast(&v.vector->Comm()); + const bool same_communicators = my_comm != NULL && v_comm != NULL && + my_comm->DataPtr() == v_comm->DataPtr(); +#else + const bool same_communicators = true; +#endif + if (!same_communicators || local_range() != v.local_range()) { Epetra_LocalMap map (global_length(*(v.vector)), v.vector->Map().IndexBase(), v.vector->Comm()); vector.reset (new Epetra_FEVector(map)); } - else + else if (omit_zeroing_entries) { int ierr; Assert (vector->Map().SameAs(v.vector->Map()) == true, diff --git a/tests/lac/vector_reinit_01.cc b/tests/lac/vector_reinit_01.cc new file mode 100644 index 0000000000..cd4e48a522 --- /dev/null +++ b/tests/lac/vector_reinit_01.cc @@ -0,0 +1,107 @@ +// --------------------------------------------------------------------- +// +// Copyright (C) 2016 by the deal.II authors +// +// This file is part of the deal.II library. +// +// The deal.II library is free software; you can use it, redistribute +// it, and/or modify it under the terms of the GNU Lesser General +// Public License as published by the Free Software Foundation; either +// version 2.1 of the License, or (at your option) any later version. +// The full text of the license can be found in the file LICENSE at +// the top level of the deal.II distribution. +// +// --------------------------------------------------------------------- + + +// check that parallel::distributed::Vector::reinit does not carry over any +// state that can lead to invalid memory access. In this test, the MPI +// communicator is deleted. + + +#include "../tests.h" +#include +#include +#include + +template +void do_test() +{ + IndexSet set(5); + set.add_range(0,5); + + VectorType v1, v2; + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v1.reinit(set, communicator); + deallog << "reinit: " << v1.size() << " "; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v2.reinit(set, communicator); + v1.reinit(v2); + deallog << v1.size() << " "; + v1.reinit(v2); + deallog << v1.size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v2.reinit(set, communicator); + v1 = v2; + deallog << "assign " << v1.size() << " "; + v1 = v2; + deallog << v1.size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + GrowingVectorMemory memory; + typename VectorMemory::Pointer v3(memory); + v1.reinit(set, communicator); + v3->reinit(v1); + deallog << "reinit pool " << v1.size() << " " << v3->size() << " "; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + GrowingVectorMemory memory; + typename VectorMemory::Pointer v3(memory); + v1.reinit(set, communicator); + v3->reinit(v1); + deallog << "reinit pool " << v3->size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + +} + +int main (int argc, char **argv) +{ + Utilities::MPI::MPI_InitFinalize mpi_initialization (argc, argv, testing_max_num_threads()); + + mpi_initlog(); + + do_test >(); +} diff --git a/tests/lac/vector_reinit_01.output b/tests/lac/vector_reinit_01.output new file mode 100644 index 0000000000..747b8affb7 --- /dev/null +++ b/tests/lac/vector_reinit_01.output @@ -0,0 +1,4 @@ + +DEAL::reinit: 5 5 5 +DEAL::assign 5 5 +DEAL::reinit pool 5 5 reinit pool 5 diff --git a/tests/lac/vector_reinit_02.cc b/tests/lac/vector_reinit_02.cc new file mode 100644 index 0000000000..9977c26879 --- /dev/null +++ b/tests/lac/vector_reinit_02.cc @@ -0,0 +1,108 @@ +// --------------------------------------------------------------------- +// +// Copyright (C) 2016 by the deal.II authors +// +// This file is part of the deal.II library. +// +// The deal.II library is free software; you can use it, redistribute +// it, and/or modify it under the terms of the GNU Lesser General +// Public License as published by the Free Software Foundation; either +// version 2.1 of the License, or (at your option) any later version. +// The full text of the license can be found in the file LICENSE at +// the top level of the deal.II distribution. +// +// --------------------------------------------------------------------- + + +// check that TrilinosWrappers(::MPI)::Vector::reinit does not carry over any +// state that can lead to invalid memory access. In this test, the MPI +// communicator is deleted. + + +#include "../tests.h" +#include +#include +#include + +template +void do_test() +{ + IndexSet set(5); + set.add_range(0,5); + + VectorType v1, v2; + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v1.reinit(set, communicator); + deallog << "reinit: " << v1.size() << " "; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v2.reinit(set, communicator); + v1.reinit(v2); + deallog << v1.size() << " "; + v1.reinit(v2); + deallog << v1.size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v2.reinit(set, communicator); + v1 = v2; + deallog << "assign " << v1.size() << " "; + v1 = v2; + deallog << v1.size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + GrowingVectorMemory memory; + typename VectorMemory::Pointer v3(memory); + v1.reinit(set, communicator); + v3->reinit(v1); + deallog << "reinit pool " << v1.size() << " " << v3->size() << " "; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + GrowingVectorMemory memory; + typename VectorMemory::Pointer v3(memory); + v1.reinit(set, communicator); + v3->reinit(v1); + deallog << "reinit pool " << v3->size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + +} + +int main (int argc, char **argv) +{ + Utilities::MPI::MPI_InitFinalize mpi_initialization (argc, argv, testing_max_num_threads()); + + mpi_initlog(); + + do_test(); + do_test(); +} diff --git a/tests/lac/vector_reinit_02.with_trilinos=true.output b/tests/lac/vector_reinit_02.with_trilinos=true.output new file mode 100644 index 0000000000..02a81b0620 --- /dev/null +++ b/tests/lac/vector_reinit_02.with_trilinos=true.output @@ -0,0 +1,7 @@ + +DEAL::reinit: 5 5 5 +DEAL::assign 5 5 +DEAL::reinit pool 5 5 reinit pool 5 +DEAL::reinit: 5 5 5 +DEAL::assign 5 5 +DEAL::reinit pool 5 5 reinit pool 5 diff --git a/tests/lac/vector_reinit_03.cc b/tests/lac/vector_reinit_03.cc new file mode 100644 index 0000000000..8112357e43 --- /dev/null +++ b/tests/lac/vector_reinit_03.cc @@ -0,0 +1,108 @@ +// --------------------------------------------------------------------- +// +// Copyright (C) 2016 by the deal.II authors +// +// This file is part of the deal.II library. +// +// The deal.II library is free software; you can use it, redistribute +// it, and/or modify it under the terms of the GNU Lesser General +// Public License as published by the Free Software Foundation; either +// version 2.1 of the License, or (at your option) any later version. +// The full text of the license can be found in the file LICENSE at +// the top level of the deal.II distribution. +// +// --------------------------------------------------------------------- + + +// check that PETScWrappers::MPI::Vector::reinit does not carry over any +// state that can lead to invalid memory access. In this test, the MPI +// communicator is deleted. + + +#include "../tests.h" +#include +#include +#include +#include + +template +void do_test() +{ + IndexSet set(5); + set.add_range(0,5); + + VectorType v1, v2; + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v1.reinit(set, communicator); + deallog << "reinit: " << v1.size() << " "; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v2.reinit(set, communicator); + v1.reinit(v2); + deallog << v1.size() << " "; + v1.reinit(v2); + deallog << v1.size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + v2.reinit(set, communicator); + v1 = v2; + deallog << "assign " << v1.size() << " "; + v1 = v2; + deallog << v1.size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + GrowingVectorMemory memory; + typename VectorMemory::Pointer v3(memory); + v1.reinit(set, communicator); + v3->reinit(v1); + deallog << "reinit pool " << v1.size() << " " << v3->size() << " "; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + + { + MPI_Comm communicator = Utilities::MPI::duplicate_communicator(MPI_COMM_WORLD); + GrowingVectorMemory memory; + typename VectorMemory::Pointer v3(memory); + v1.reinit(set, communicator); + v3->reinit(v1); + deallog << "reinit pool " << v3->size() << std::endl; + +#ifdef DEAL_II_WITH_MPI + MPI_Comm_free (&communicator); +#endif + } + +} + +int main (int argc, char **argv) +{ + Utilities::MPI::MPI_InitFinalize mpi_initialization (argc, argv, 1); + + mpi_initlog(); + + do_test(); +} diff --git a/tests/lac/vector_reinit_03.with_petsc=true.output b/tests/lac/vector_reinit_03.with_petsc=true.output new file mode 100644 index 0000000000..747b8affb7 --- /dev/null +++ b/tests/lac/vector_reinit_03.with_petsc=true.output @@ -0,0 +1,4 @@ + +DEAL::reinit: 5 5 5 +DEAL::assign 5 5 +DEAL::reinit pool 5 5 reinit pool 5 -- 2.39.5