Pass pointer data across multiple functions

Question

I would like review on the following dynamic memory allocation process and suggestions on whether there are any memory leaks. Following code is not real code in use, but I'm trying to understand concepts of memory allocations and de-allocation in different ways.

class ObjMapData
{
private:
//Header Info fields to describe each object actual data type, total elements of actual data type, and lot more describing the object common fields
int itsDataTypeID; // 1 to 20+, each represent one type of data being used
int itsDataType; // 1 to 8, which could be UINT8, INT8, UINT16, INT16, UINT32, INT32
int itsDataSize; // 1 or 2 or 4 bytes
int itsTotalElements; // original data type elements, but not the total bytes of the binary file
//...... other common header information
// body data, (actual data could be UINT8, INT8, UINT16, INT16, UINT32, INT32 type)
char* itsMapData; // size = itsTotalElements*itsDataSize; type cast to various actual data types later to access/populate the data
........
public:
ObjMapData();
ObjMapData& operator = (const ObjMapData& objMyMap);
ObjMapData(const ObjMapData& objMyMap);
~ObjMapData(){if(itsMapData!= NULL) delete[] itsMapData;}
//Following two functions are used to allocate/free memory within the scope of the object so that it can be re-used again for a different data type object
ClearMemory() {if(itsMapData!= NULL) {delete[] itsMapData; itsMapData= NULL}}
void AllocateMemory( unsigned long  nSizeInBytes = 0)
{
if(itsMapData!= NULL)   {delete[] itsMapData; itsMapData= NULL;}
itsMapData= new (std::nothrow) INT8[nSizeInBytes];
}
.......
void SetMapData(char* ptrData) { itsMapData = ptrData;} // or should I use char*&ptrData??
char* GetMapData() const { return itsMapData;}
}

Now can I do the following without any memory leaks?

//There are 30+ functions like Function1() to populate the map data of different type of maps using different sources of input data or algorithms
bool Function1(ObjMapData& objMyMap)
{
//populate the ptrData with some data values using many different ways
int* ptrData = new int[1000]; // usually a variable from binary file header

......................
objMyMap.SetMapData((char*)ptrData);
//don't want to delete the memory yet as this map will be used along with map objects to create a new mapdata 
return true;
}

bool Function2(ObjMapData& objMyMap)
{
int* ptrData = (int*) objMyMap.GetMapData();
//do some work such as writing updated data and then write into a binary file
}

bool Function3(ObjMapData& objMyMap)
{
//populate the data using different method for different map data types
//there could be 30+ other functions like Function1() to populate the map data
bool bStatus = Function1(objMyMap);

int* ptrData = (int*)objMyMap.GetMapData();
//update the map data by modifying ptrData variable
..........
objMyMap.SetMapData((char*)ptrData); // Do I need to set again or member pointer get updated automatically?
bStatus = Function2(objMyMap); // write data to a binary file
objMyMap.ClearMemory(); // not real code  in use, but for understanding the concept
bStatus = Function1(objMyMap); // re-allocate memory and assign a new Map Data
short* ptrData2 = (short*) objMyMap.GetMapData();
//update the map data using ptrData variable
objMyMap.SetMapData((char*)ptrData2); // Do I need to set again or member pointer get updated automatically?
return true
}
int main()
{
ObjMapData objMyMap;
bool bStatus = Function3(objMyMap);
short* ptrData = (short*)objMyMap.GetMapData();
//do something with ptrData
//default destructor of objMyMap can take care of the allocated memory cleanup
return 0;
}

Answer 1

Don't do your own memory management. Use the tools provided for you unless you have good reason to do otherwise.

This:

objMyMap.SetMapData((char*)ptrData);

causes undefined behavior. This is because when objMyMap is deleted you delete the pointer as a char pointer (it was created as an int pointer). Thus the runtime memory management will probably get the size of the block wrong.

If you ever use a C-Cast then you are probably doing something wrong. Always use C++ style casts. Then it is easy to spot when you are doing things wrong.

const_cast<>()          // Bad if you casting away constness.
reinterpret_cast<>()    // Bad unless used to interact with C libraries.
static_cast<>()         // Bad if used to cast pointers around
dynamic_cast<>()        // Safe but slow as it is a runtime cast.
                        // Note: Only useful for casting down class hierarchy
                        //       But it is also a sign of bad design.

Looking at the class.

class ObjMapData
{

There is no reason do your own memory management.

private:
char* itsMapData;

This should probably be a std::vector<char>. Once you do that the rest becomes irelavant and all memory management issues are solved.

........
public:
ObjMapData();

Don't bother to test for NULL. Just delete.

~ObjMapData(){if(itsMapData!= NULL) delete[] itsMapData;}
ClearMemory() {if(itsMapData!= NULL) {delete[] itsMapData; itsMapData= NULL}}
.......

This obviously leaks the memory that you had before.

void SetMapData(char* ptrData) { itsMapData = ptrData;} // or should I use int*&ptrData??

Sure. But only if you need to expose that data. I would rather encapsolate the manipulation of the data inside the class so you don't need to expose it.

char* GetMapData() const { return itsMapData;}
}

You don't show it so I have to assume you are not following the "Rule of Three" (look it up).

I would have done this:

class ObjMapData
{
    private:
        std::vector<char> itsMapData;

    public:
        clearMemory() // lower case first letter of non type.
        {
            itsMapData.swap(std::vector<char>());
        }
        void setMapData(std::vector<char>& ptrData)
        {
             itsMapData.swap(ptrData);
        }
        char* getMapData() const { return &itsMapData[0];}
};

Addition based on comment:

(could be INT8, UINT8, INT16, UINT16, UINT32, INT32)

// Note the above data is not correctly aligned on all systems.

// INT8   INT8      INT16            INT16

01234567  01234567  0123456701234567 0123456701234567  Not a 32 Bit Boundary
|         |         |       |        |       |         |
8/16/32 Boundary    8/16 Boundary    8/16/32 Boundary  8/16 Boundary
          8 Boundary        8 Boundary       8 Boundary

But lets assume you just made a typo and you know what you are doing.

// Need room for
// INT 8 | INT 8 | INT 16 | INT 32 | INT 32
std::vector<char>  data(96);
::read(fd, &data[0], 96);    // Wow look at that fits neatly.

INT8  d1 = *reinterpret_cast<INT8 *>(&data[ 0]);  // The reinterpret cast is required
INT8  d2 = *reinterpret_cast<INT8 *>(&data[ 8]);  // It also shows its really dangerious
INT16 d3 = *reinterpret_cast<INT16*>(&data[16]);
INT32 d4 = *reinterpret_cast<INT16*>(&data[32]);
INT32 d5 = *reinterpret_cast<INT32*>(&data[64]);

Answer 2

Short Answer: Yes

Longer Answer: As far as I see, your code works without any memory leaks. A program which helps you detect memleaks would be valgrind.

With this code you have to be very careful. I recommend checking the pointer before assigning it in SetMapData(int* ptrData). Something like:

void SetMapData(int* ptrData) // or should I use int*&ptrData?? -> no
{
    if (itsMapData != NULL && itsMapData != ptrData)
    {
        delete[] itsMapData;
    } 
    itsMapData = ptrData;
} 

If you add these lines you don't have to manually call ClearMemory() before assigning new MapData.

Answer 3

SetMapData will not leak on first invocation, it will leak on ayny consecutive invocation. Its a tricky one to get right in multi-threaded settings b.t.w, lots of non-obvious ways to introduce TOCTOU issues. Setter based resource injection is tricky in C++ for numerous reasons. I would suggest you'll try to avoid using this idiom and use good old constructor injected RAII instead whenever possible.

Answer 4

Although your client code has no memory leaks, there are a few design problems with your class.

As other answers mention, the interface of the class allows you to use it incorrectly and create memory leaks.

Code:

ObjMapData objMyMap;
objMyMap.SetMapData(new char[20]); // leaked when executing next statement
objMyMap.SetMapData(new char[20]);

The implementation makes bad assumptions how the class will be used.

ObjMapData objMyMap;
objMyMap.SetMapData(new char{'a'}); // UB: will be deleted using delete[]

You use C-style casts for casting the data. Don't do that - it's unsafe code.