Heap corruption while freeing memory

Question

I have a class as follows

 struct CliHandler {
     CliHandler(int argc, char** argv);
     ~CliHandler();

     int doWork();

     int argc_; 
     char** argv_;  
     private:
     CliHandler(const CliHandler&){}
     CliHandler& operator=(const CliHandler&){} 
 };

//Constructor

 CliHandler::CliHandler(int argc,
 char** argv) {
     //set command line parameters
     argc_ = argc; 

     argv_ = (char**) malloc(argc_ * sizeof(char*));

     for(int i=0; i<argc_; ++i)
     {
         std::cout<<sizeof(argv[i]); 
         argv_[i] = (char*) malloc(strlen(argv[i]) *
 sizeof(char));
         StrCpy(argv_[i], argv[i]);
     } }

// destructor

 CliHandler::~CliHandler() {
     for(int i=0; i<argc_; ++i)
         free(argv_[i]); 
     free(argv_);  }

While I debug, I get an error " Heap corruption detected. CRT detected that application wrote to memory after end of heap buffer. " My question id "Where exactly am i making a mistake ? How do I fix it". I am using visual stdio 2008.

Edit:I did something like this to add 1

argv_[i] = (char*) malloc(strlen(argv[i] + 1) * sizeof(char));

Which is terrible as it increments the pointer argv[i] by one. My co-worker pointed out that subtle issue. It should be

argv_[i] = (char*) malloc( (strlen(argv[i]) + 1) * sizeof(char));

One thing that I see is that you don't allocate space for the null-terminator. Should be strlen(argv[i]) + 1. — Marius Bancila
Why, if you are using C++, are you using malloc? And why are you not using std:;vector and std::string? — user2100815
Another potential problem I see is that you don't follow the Rule of Three. If any copying happens, you're in trouble. — Fred Larson
@unapersson - i'm integrating some function with legacy code. — Eternal Learner
So what? The things you allocate to are private, so they can't be used directly by the legacy code, so you could (and should) implement them using vectors and strings. Otherwise, you are simply writing MORE legacy code. — user2100815

nos nos · Accepted Answer · 2011-05-09T19:43:05

Change the code to:

 argv_[i] = (char*) malloc(strlen(argv[i]) + 1) ; 
 strcpy(argv_[i], argv[i]);

It's because your StrCpy likely trashes your memory. You have to account for the terminating nul byte as well, provided your StrCpy works like the standard strcpy (which it has to in order to be useful, better just use the standard strcpy() unless you have a good reason not to).

sizeof(char) is by definition 1, so that can be omitted too.

Heap corruption while freeing memory

4 Answers