delete dynamic memory in assignment operator on uninitialized array member

[nll2010]

Hi:

I'm using visual studio 2010 and doing a Win32 (native) C++ exercise. (Ivor Horton's 2010 p.793)
The goal is to prevent all memory leaks.

The suggested code, from the book:

Ex11_02A.cpp

// Ex11_02.cpp : Extending the test operation
#include <iostream>
#include <stdlib.h>
#include <crtdbg.h>
#include <cstdio>
#include "Name.h"
using namespace std;

// Function to initialize an array of random names
void init(Name* names, int count)
{
  char* firstnames[] = { "Charles", "Mary", "Arthur", "Emily", "John"};
  int firstsize = sizeof (firstnames)/sizeof(firstnames[0]);
  char* secondnames[] = { "Dickens", "Shelley", "Miller", "Bronte", "Steinbeck"};
  int secondsize = sizeof (secondnames)/sizeof(secondnames[0]);
  char* first = firstnames[0];
  char* second = secondnames[0];

  for(int i = 0 ; i<count ; i++)
  {
    if(i%2)
      first = firstnames[i%firstsize];
    else
      second = secondnames[i%secondsize];

 	*(names+i) = Name(first, second);
  }
}

int main(int argc, char* argv[])
{
  FILE *pOut(nullptr);
  errno_t err = freopen_s( &pOut, "debug_out.txt", "w", stdout );

  if(err)
	  cout << "error on freopen" << endl;
  
  _CrtSetDbgFlag( _CRTDBG_LEAK_CHECK_DF | _CRTDBG_ALLOC_MEM_DF );

  _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
  _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDOUT);

  Name myName("Ivor", "Horton");                 // Try a single object

  // Retrieve and store the name in a local char array  
  char theName[12];
  cout << "\nThe name is " << myName.getName(theName);

  // Store the name in an array in the free store
  char* pName = new char[myName.getNameLength()+1]; 
  cout << "\nThe name is " << myName.getName(pName);

  const int arraysize = 5;
  Name names[arraysize];                          // Try an array

  // Initialize names
  init(names, arraysize);

  // Try out comparisons
  char* phrase = nullptr;                         // Stores a comparison phrase
  char* iName = nullptr;                          // Stores a complete name  
  char* jName = nullptr;                          // Stores a complete name  

  for(int i = 0; i < arraysize ; i++)             // Compare each element
  {
    iName = new char[names[i].getNameLength()+1]; // Array to hold first name
    for(int j = i+1 ; j<arraysize ; j++)          // with all the others
    {
      if(names[i] < names[j])
        phrase = " less than ";
      else if(names[i] > names[j])
        phrase = " greater than ";
      else if(names[i] == names[j])     // Superfluous - but it calls operator==() 
        phrase = " equal to ";

      jName = new char[names[j].getNameLength()+1]; // Array to hold second name
      cout << endl << names[i].getName(iName) << " is" << phrase 
           << names[j].getName(jName);

	  delete[] jName;
    }

	delete[] iName;
  }

  delete[] pName;

  cout << endl;
  return 0;
}

DebugStuff.h

// DebugStuff.h - Debugging control
#pragma once

#ifdef _DEBUG

//#define CONSTRUCTOR_TRACE         // Output constructor call trace
//#define FUNCTION_TRACE            // Trace function calls

#endif 

Name.h

// Name.h – Definition of the Name class
#pragma once

// Class defining a person’s name
class Name
{
public:
  Name();                                        // Default constructor
  Name(const char* pFirst, const char* pSecond); // Constructor
  Name(const Name& rname);

  ~Name();

  Name& operator = (const Name& rName);
  //Name& operator = (Name&& rName);

  char* getName(char* pName) const;              // Get the complete name
  size_t getNameLength() const;                  // Get the complete name length

  // Comparison operators for names    
   bool operator<(const Name& name) const;
   bool operator==(const Name& name) const;
   bool operator>(const Name& name) const;

private:
  char* pFirstname;
  char* pSurname;
};

Name.cpp

// Name.cpp – Implementation of the Name class
#include "Name.h"                                // Name class definitions
#include "DebugStuff.h"                          // Debugging code control
#include <cstring>                               // For C-style string functions
#include <cassert>                               // For assertions
#include <iostream>
using namespace std;

// Default constructor
Name::Name()
{
#ifdef CONSTRUCTOR_TRACE
  // Trace constructor calls
  cerr << "\nDefault Name constructor called.";
#endif

  pFirstname = new char[1];
  pSurname = new char[1];
  pFirstname = pSurname = "\0";
}

// Constructor
Name::Name(const char* pFirst, const char* pSecond)
{
  // Verify that arguments are not null
  assert(pFirst);
  assert(pSecond);

#ifdef CONSTRUCTOR_TRACE
  // Trace constructor calls
  cout << "\nName constructor called.";
#endif
  pFirstname = new char[strlen(pFirst)+1];
  strcpy(pFirstname, pFirst);
  pSurname = new char[strlen(pSecond)+1];
  strcpy(pSurname, pSecond);
}

Name::Name(const Name& rName)
{
  pFirstname = new char[strlen(rName.pFirstname)+1];
  strcpy(pFirstname, rName.pFirstname);
  pSurname = new char[strlen(rName.pSurname)+1];
  strcpy(pSurname, pSurname);
}

Name::~Name()
{
	delete[] pFirstname;
	delete[] pSurname;

}

Name& Name::operator = (const Name& rName)
{
	if(this == &rName)
		return *this;

	delete[] pFirstname;
	delete[] pSurname;

	pFirstname = new char[strlen(rName.pFirstname)+1];
	strcpy(pFirstname, rName.pFirstname);
	pSurname = new char[strlen(rName.pSurname)+1];
	strcpy(pSurname, pSurname);

	return *this;
}

// Return a complete name as a string containing first name, space, surname
// The argument must be the address of a char array sufficient to hold the name
char* Name::getName(char* pName) const
{
  assert(pName);                                 // Verify non-null argument

#ifdef FUNCTION_TRACE
  // Trace function calls
  cout << '\n' << __FUNCTION__ << " called.";
#endif

  strcpy(pName, pFirstname);                          // copy first name
  strcat(pName, " ");                                 // Append a space

  // Append second name and return total
  return strcat(pName, pSurname);      // Append second name and return total
} 

 // Returns the total length of a name
size_t Name::getNameLength() const
{
#ifdef FUNCTION_TRACE
  // Trace function calls
  cout << '\n' << __FUNCTION__ << " called.";
#endif
  return strlen(pFirstname)+strlen(pSurname)+1;
}

// Less than operator
bool Name::operator<(const Name& name) const
{
  int result = strcmp(pSurname, name.pSurname);
  if(result < 0)
    return true;
  if(result == 0 && strcmp(pFirstname, name.pFirstname) < 0)
    return true;
  else
    return false;
}

// Greater than operator
bool Name::operator>(const Name& name) const
{
  return name < *this;
}

// Equal to operator
bool Name::operator==(const Name& name) const
{
  if(strcmp(pSurname, name.pSurname) == 0 && strcmp(pFirstname, name.pFirstname) == 0)
    return true;
  else
    return false;
}

But, this results in "Debug assertion failed! ... File: f:\dd\vctools\crt_bld\self_x86\crt\src\dbgdel.cpp Line 52 Expression: _BLOCK_TYPE_IS_VALID(pHead->nBlockUse)"

this happens in the assignment operator

Name& Name::operator = (const Name& rName)
...
delete[] pFirstname;

which seems to be because there isn't anything to delete, seeing as the array is uninitialized (?)


but, if i comment out

	delete[] pFirstname;
	delete[] pSurname;

from the assignment operator, then the debug_out file indicates memory leaks!


In general, does delete-ing make sense in the assignment operator, since we might be assigning to an uninitialized instance? Or am I totally misunderstanding?

Sorry for the newbie question... and I'd be really grateful for any and all help!

[Gaminic]

It does make sense to delete them: they're pointing to a memory block specifically made for them. If you don't delete them, the pointers will be redirected and the old location will just float away unpointed.

The problem is (I'm assuming) that the first time the operator is called, the two pointers were uninitialized (pointing nowhere), so you're deleting... something, somewhere.

I'm not sure if there's an official way to check. What I do usually do is make sure pointers are initialized as 0 (or 'nullptr'). Then, check for that value before deleting:

if (p != nullptr) { delete p; p = nullptr; }
If you're consistent in making sure p is assigned 0/nullptr when it's not pointing anywhere, this should solve your problems.

[coder777]

In general, does delete-ing make sense in the assignment operator, since we might be assigning to an uninitialized instance?

delete makes sense otherwise you have what you experienced a memory leak. The variables shouldn't and aren't unitialized.

A problem occurs on line 65: strcpy(pSurname, pSurname);
The content of pSurname is indeed unintialized. it is undefined what happens here. Maybe it schouldn't do anything because it points to the same thing which would lead to problems later since then it remains uninitalized (the content and not the pointer).

Another problem is on line 72 since you cannot tell whether the variable is large enough to hold the resulting string.

[guestgulkan]

This constructor doesn't look good

Name::Name()
{
#ifdef CONSTRUCTOR_TRACE
  // Trace constructor calls
  cerr << "\nDefault Name constructor called.";
#endif

  pFirstname = new char[1];
  pSurname = new char[1];
  pFirstname = pSurname = "\0";  //both pointers now changed.
}

The pFirstname and pSurname are inititialise to some new memory on the heap;
Both pointers are then changed to point to some non-heap allocated memory.

That will cause problems (as you have found out)
1. memory leak;
2. deleteing non-heap allocated memory in the destructor.

See also previous comment by coder777 and Gaminic

/*****
OK - I had a look at the IVOR HORTONS book and it looks you have copied the code for the default constructor incorrectly.
My copy of the book says:

  pFirstname = new char[1];
  pSurname = new char[1];
  pFirstname[0] = pSurname[0] = "\0"; 

that makes more sense - why would Ivor have provided a bad constructor ?? :-) ***/

[nll2010]

Hi all:

Thanks so much! Guestgulkan - you were right, I copied it wrongly.
pFirstname = pSurname = "\0";
was causing the problem
pFirstname[0] = pSurname[0] = "\0";
works perfectly.

The array does get initialized - according to the default constructor. But, I'd made a typo in the default constructor, and that was the root of the problem.

Thank you so much Coder777 and Gaminic also - I do really appreciate your help.

[cire]

pFirstname[0] = pSurname[0] = "\0";
works perfectly.

No, it doesn't. "\0" is interpreted in this context as a pointer to the string literal (which is almost definitely non-zero.) Since the idea here is to signal the end of a C style string, this just won't get the job done (and I'm sure your compiler warns you of at least a type mismatch here.)

pFirstname[0] = pSurname[0] = '\0' ;

[guestgulkan]

Sorry about that - the error is in my typing not the book - the book certainly used single quotes -

pFirstname[0] = pSurname[0] = '\0'; //as I should have written