Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a memory leak in AlignedVector. #13005

Merged
merged 2 commits into from
Nov 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions include/deal.II/base/aligned_vector.h
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ namespace internal


/**
* A class that given a range of memory locations calls either calls
* A class that given a range of memory locations either calls
* the placement-new operator on these memory locations (if
* `initialize_memory==true`) or just copies the given initializer
* into this memory location (if `initialize_memory==false`). The
Expand Down Expand Up @@ -1199,11 +1199,22 @@ template <class T>
inline AlignedVector<T> &
AlignedVector<T>::operator=(const AlignedVector<T> &vec)
{
const size_type new_size = vec.used_elements_end - vec.elements.get();

// First throw away everything and re-allocate memory but leave that
// memory uninitialized for now:
resize(0);
resize_fast(vec.used_elements_end - vec.elements.get());
reserve(new_size);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is here: When we call resize_fast(), we allocate memory and default construct objects in that memory. But in the line below (AlignedVectorCopyConstruct) we copy-construct other objects in that same memory location, forgetting to call the destructor on what we had done before.

The solution is to not initialize the memory we allocate -- that's what reserve() does in contrast to resize*().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, on second look, I see that in essence the same code I just wrote here is also what's in the copy constructor just a few lines up.


// Then copy the elements over by using the copy constructor on these
// elements:
internal::AlignedVectorCopyConstruct<T>(vec.elements.get(),
vec.used_elements_end,
elements.get());

// Finally adjust the pointer to the end of the elements that are used:
used_elements_end = elements.get() + new_size;

return *this;
}

Expand Down
109 changes: 109 additions & 0 deletions tests/base/aligned_vector_memory_02.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
// --------------------------------------------------------------------------------------------
//
// Copyright (C) 2021 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.md at
// the top level directory of deal.II.
//
// --------------------------------------------------------------------------------------------


// Check for a bug in memory management in class Table (which uses
// AlignedVector) that had crept in at some point.

#include <deal.II/base/logstream.h>
#include <deal.II/base/table.h>

#include "../tests.h"


int object_number = 0;
int objects_destroyed = 0;

class C
{
public:
C()
{
object_number = ::object_number++;
deallog << "default constructor. Object number " << object_number
<< std::endl;
}

C(const C &c)
{
object_number = ::object_number++;
deallog << "copy constructor from " << c.object_number << ". Object number "
<< object_number << std::endl;
}

C(const C &&c)
{
object_number = ::object_number++;
deallog << "move constructor from " << c.object_number << ". Object number "
<< object_number << std::endl;
}

C &
operator=(const C &c)
{
deallog << "copy operator called for " << object_number << " <- "
<< c.object_number << std::endl;
return *this;
}

C &
operator=(const C &&c)
{
deallog << "move operator called for " << object_number << " <- std::move("
<< c.object_number << ')' << std::endl;
return *this;
}


~C()
{
deallog << "destructor. Object number " << object_number << std::endl;
++objects_destroyed;
}

private:
unsigned int object_number;
};



void
test()
{
deallog << "---- Creating outer table" << std::endl;
Table<1, C> table({1});

// Copy the object, then destroy the copy again.
{
deallog << "---- Cloning outer table" << std::endl;
Table<1, C> x(table);
deallog << "---- Destroying the clone" << std::endl;
}

deallog << "---- Destroying the source table" << std::endl;
}



int
main(int argc, char **argv)
{
initlog();

test();

deallog << "Objects created: " << object_number << std::endl;
deallog << "Objects destroyed: " << objects_destroyed << std::endl;
}
13 changes: 13 additions & 0 deletions tests/base/aligned_vector_memory_02.output
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

DEAL::---- Creating outer table
DEAL::default constructor. Object number 0
DEAL::---- Cloning outer table
DEAL::default constructor. Object number 1
DEAL::destructor. Object number 1
DEAL::copy constructor from 0. Object number 2
DEAL::---- Destroying the clone
DEAL::destructor. Object number 2
DEAL::---- Destroying the source table
DEAL::destructor. Object number 0
DEAL::Objects created: 3
DEAL::Objects destroyed: 3