Skip to content

Fixed double-free memory corruption. #318

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.

Sign up for GitHub

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

Open
karya0 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fixed double-free memory corruption. #318

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 18 additions & 29 deletions mpi-proxy-split/record-replay.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <algorithm>
#include <functional>
#include <memory>
#include <mutex>
#include <typeinfo>
#include <unordered_map>
Expand Down Expand Up @@ -106,34 +107,22 @@ namespace dmtcp_mpi
// Struct for saving arbitrary function arguments
struct FncArg
{
void *_data;
std::shared_ptr<void> _data;
enum TYPE _type;

FncArg(const void *data, size_t len, dmtcp_mpi::TYPE type)
: _data(JALLOC_HELPER_MALLOC(len))
// Default _type set to TYPE_INT_ARRAY because this constructor is also used
// by CREATE_LOG_BUF in MPI_Cart functions.
FncArg(const void *data, size_t len, dmtcp_mpi::TYPE type = TYPE_INT_ARRAY)
Copy link
Collaborator

@gc00 gc00 May 31, 2023

Choose a reason for hiding this comment

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

Thanks for consolidating these two constructors with an optional argument. This is definitely clearer. But wouldn't it be even better if we changed:

#define CREATE_LOG_BUF(buf, len) dmtcp_mpi::FncArg(buf, len)
->
`#define CREATE_LOG_BUF(buf, len) dmtcp_mpi::FncArg(buf, len, TYPE_INT_ARRAY)

Then we don't need an optional argument, and the logic of the code is more local.
`

: _type(type)
{
_type = type;
if (_data && data) {
memcpy(_data, data, len);
void *ptr = JALLOC_HELPER_MALLOC(len);
if (ptr) {
_data = std::shared_ptr<void>(ptr,
Copy link
Collaborator

@gc00 gc00 May 31, 2023

Choose a reason for hiding this comment

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

I suggest an added comment here:

          // FIXME:  This 'shared_ptr' fixes an apparent double-free.
           //   However, this is a temporary fix, until we can discover
           //     why this happens.
```

[](void *p) { JALLOC_HELPER_FREE(p);});
}
Copy link
Collaborator

@gc00 gc00 May 31, 2023

Choose a reason for hiding this comment

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

In the if (ptr), shouldn't we add an else clause to assert or panic? If JALLOC_HELPER_MALLOC() failed, then we want to know this now.

}

// This constructor is only used by CREATE_LOG_BUF
FncArg(const void *data, size_t len)
: _data(JALLOC_HELPER_MALLOC(len))
{
// Default _type set to TYPE_INT_ARRAY because this constructor is used
// by CREATE_LOG_BUF in MPI_Cart functions.
_type = dmtcp_mpi::TYPE_INT_ARRAY;
if (_data && data) {
memcpy(_data, data, len);
}
}

~FncArg()
{
if (!_data) {
Copy link
Collaborator

@gc00 gc00 May 31, 2023

Choose a reason for hiding this comment

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

This doesn't matter now that we are revising this code. But isn't if (!_data) a separate original bug (a memory leak)?

First of all, _data will always be a valid pointer. All versions of the constructor allocated a _data buffer. Maybe this logic is left over from an earlier design. So, it seems like !_data will always be false, and so we never free _data. This is a memory leak.

JALLOC_HELPER_FREE(_data);
memcpy(_data.get(), data, len);
}
}

Expand All @@ -149,7 +138,7 @@ namespace dmtcp_mpi
// Returns an int corresponding to the saved argument
operator int() const
{
return *(int*)_data;
return *(int*)_data.get();
}

// Returns an int pointer to saved argument
Expand All @@ -160,9 +149,9 @@ namespace dmtcp_mpi
operator int*() const
{
if (_type == dmtcp_mpi::TYPE_INT_ARRAY) {
return (int*)_data;
return (int*)_data.get();
} else if (_type == dmtcp_mpi::TYPE_INT_PTR) {
return *(int**)_data;
return *(int**)_data.get();
} else {
JASSERT(false).Text("Unsupported arg type");
return NULL;
Expand All @@ -173,24 +162,24 @@ namespace dmtcp_mpi
// MPI_Aint is 8 bytes (long int).
operator long*() const
{
return (long*)_data;
return (long*)_data.get();
}

// Returns a void pointer to saved argument
operator void*() const
{
return *(void**)_data;
return *(void**)_data.get();
}

// Returns a void pointer to saved argument
operator void const*() const
{
return *(void const**)_data;
return *(void const**)_data.get();
}

operator MPI_User_function*() const
{
return *(MPI_User_function**)_data;
return *(MPI_User_function**)_data.get();
}
};

Expand Down