Garbage with pointers in a class, C++.

Question

I am using Borland Builder C++. I have a memory leak and I know it must be because of this class I created, but I am not sure how to fix it. Please look at my code-- any ideas would be greatly appreciated!

Here's the .h file:

#ifndef HeaderH
#define HeaderH
#include <vcl.h>
#include <string>
using std::string;
class Header {

 public:
    //File Header
    char FileTitle[31];
    char OriginatorName[16];

    //Image Header
    char ImageDateTime[15];
    char ImageCordsRep[2];
    char ImageGeoLocation[61];

    NitfHeader(double latitude, double longitude, double altitude, double heading);
    ~NitfHeader();
    void SetHeader(char * date, char * time, double location[4][2]);  


 private:

    void ConvertToDegMinSec (double angle, AnsiString & s, bool IsLongitude);
    AnsiString ImageDate;
    AnsiString ImageTime;
    AnsiString Latitude_d;
    AnsiString Longitude_d;
    double Latitude;
    double Longitude;
    double Heading;
    double Altitude;

};

And here is some of the .cpp file:

void Header::SetHeader(char * date, char * time, double location[4][2]){
    //File Header
strcpy(FileTitle,"Cannon Powershot A640");
strcpy(OperatorName,"Camera Operator");

   //Image Header
//Image Date and Time
   ImageDate = AnsiString(date);
   ImageTime = AnsiString(time);
   AnsiString secstr = AnsiString(ImageTime.SubString(7,2));
   AnsiString rounder = AnsiString(ImageDate.SubString(10,1));
   int seconds = secstr.ToInt();
    //Round off seconds  - will this be necessary with format hh:mm:ss in text file?
   if (rounder.ToInt() > 4) {
     seconds++;
    }
   AnsiString dateTime = ImageDate.SubString(7,4)+ ImageDate.SubString(4,2) + ImageDate.SubString(1,2) + ImageTime.SubString(1,2)
     + ImageTime.SubString(4,2) + AnsiString(seconds);
   strcpy(ImageDateTime,dateTime.c_str());

   //Image Coordinates Representation
   strcpy(ImageCordsRep,"G");

   //Image Geographic Location
   AnsiString lat;
   AnsiString lon;
   AnsiString locationlat_d;
   AnsiString locationlon_d;
   AnsiString corner;

   for (int i = 0; i < 4; i++){

     ConvertToDegMinSec(location[i][0],lat,false);
     ConvertToDegMinSec(location[i][1],lon,true);

     if(location[i][0] < 0){
     locationlat_d = 'S';
     ConvertToDegMinSec(-location[i][0],lat,false);
      }else if(location[i][0] > 0){
     locationlat_d = 'N';
     }else locationlat_d = ' ';

     if(location[i][1] < 0){
     locationlon_d = 'W';
     ConvertToDegMinSec(-location[i][1],lon,true);
     }else if(location[i][1] > 0){
      locationlon_d = 'E';
     }else locationlon_d = ' ';

     corner += lat + locationlat_d + lon + locationlon_d;

   }
   strcpy(ImageGeoLocation,corner.c_str());

}

Now when I use the class in main, basically I just create a pointer:

Header * header = new Header;
header->SetHeader(t[5],t[6],corners->location);
char * imageLocation = header->ImageGeoLocation;
//do something with imageLocation
delete header;

Where corners->location is a string from another class, and t[5] and t[6] are both strings. The problem is that imageLocation doesn't contain what is expected, and often just garbage. I have read a lot about memory leaks and pointers, but I am still very new to programming and some of it is quite confusing. Any suggestions would be fabulous!!

Answer 1

Your memory leak is in main; you are making a pointer with new, but not subsequently calling delete.

If you wish to just create an object of type Header that will be destroyed when main exits, just declare it as "Header header;" If you wish to create a persistent pointer, you should use new as you do, but be sure to delete header; and some point prior to the program ending.

Answer 2

I'm afraid there are a number of issues here.

For starters char ImageCordsRep[1]; doesn't work ... a string is always null terminated, so when you do strcpy(ImageCordsRep,"G"); you are overflowing the buffer.

It would also be good practice to terminate all those string buffers with a null in your constructor, so they are always valid strings.

Even better would be to use a string class instead of the char arrays, or at least use 'strncpy' to prevent buffer overruns if the incoming strings are larger than you expect.

Answer 3

Something else...

Be careful not to use imageLocation after the header object is deleted. It's often better to copy the string from the object instead of getting a pointer to it. It could be OK in this case depending on the rest of the code.

Header * header = new Header;
header->SetHeader(t[5],t[6],corners->location);
char * imageLocation = header->ImageGeoLocation;

Answer 4

Is your problem that ImageGeoLocation is trash or you have a memory leak?

If you code is written as such:

Header * header = new Header;
header->SetHeader(t[5],t[6],corners->location);
char * imageLocation = header->ImageGeoLocation;
delete header;
printf("ImageLocation is %s", imageLocation);

Then you problem isn't a memory leak, but that you are deleting the memory out from under imageLocation. ImageLocation is just a pointer and doesn't actually contain data, it just points to it. So if you delete the data, then the pointer is pointing to trash.

If that isn't the case, then debug your SetHeader method. Is ImageGeoLocation getting populated with data as you expect? If it is, then imageLocation must point to valid data unless there is some omitted code that is damaging ImageGeoLocation later on. A memory what window looking at ImageGeoLocation can help since you will be able to step through your code and see which line actually changes ImageGeoLocation where you don't expect.

Answer 5

Thank you, Torlack, and others for replying so quickly. Basically, imageLocation gets populated fine, unless I have other code before it. For example, I have this string list, which basically contains file names.

    AnsiString fileType ("*.jpg");
 AnsiString path = f + fileType;
 WIN32_FIND_DATA fd;
 HANDLE hFindJpg = FindFirstFile(path.c_str(),&fd);

   //Find all images in folder
 TStringList * imageNames = new TStringList;

 if (hFindJpg != INVALID_HANDLE_VALUE) {
  do{

   if(!(fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)){
    image = AnsiString(fd.cFileName);
    imageNames->Add(image);

    jpgFileCount++;
   }

  }while(FindNextFile(hFindJpg,&fd));
 }else ShowMessage ("Cannot find images.");

 FindClose(hFindJpg);

Now when I try to refer to an image from the list directly before, I get the name of the image put inside imageLocation.

 //char * imageLocation = header->ImageGeoLocation; //as expected
Image1->Picture->LoadFromFile(imageNames->Strings[j]);
char * imageLocation = header->ImageGeoLocation; //puts name of jpg file in imageLocation

Answer 6

So I changed strcpy() to strncpy() and it got rid of the garbage! Thank you everyone!