C : Double free or corruption, can't find error

Question

This error is making me mad...

Valgrinding the program shows that the delete[] in DataPackage::~DataPackage (line 40) creates the problem. But if I remove it, the program will leak. So, how to repair it and what have I done wrong?

main.cxx

#include "main.h" // currently includes DataPackage.h only

DataPackage aTestFunction(){
  return DataPackage("hello",5);
}

int main(){
  DataPackage pack1 = aTestFunction(), pack2 = aTestFunction();
  pack1 = pack2;

  for(unsigned int i = 0; i < pack1.getLength(); i++){
    printf("0x%02x ", *(pack1.getData()+i)&0xff);
  }

  return 0;
}

DataPackage.cxx

#include "DataPackage.h" // defines only the class, and private members m_data (char*) and m_length (size_t), includes <cstdio> and <cstring>

DataPackage::DataPackage(){
  m_data = NULL;
  m_length = 0;
}

DataPackage::DataPackage(string data){
  m_data = NULL;
  setData(data.c_str(),data.length()+1);
}

DataPackage::DataPackage(const char *data, size_t length) {
  m_data = NULL;
  setData(data,length);
}

DataPackage::DataPackage(const DataPackage &pack) {
  m_data = NULL;
  setData(pack.m_data,pack.m_length);
}

const char* DataPackage::getData(){
  return m_data;
}

void DataPackage::setData(const char *newdata,size_t newlength){
  char* tmpdata = new char[newlength];
  m_length = newlength;
  memcpy(tmpdata,newdata,m_length);
  delete[] m_data;
  m_data = tmpdata;
}

size_t DataPackage::getLength(){
  return m_length;
}

DataPackage::~DataPackage() {
  delete[] m_data;
}

score 3 · Answer 1 · edited May 23 '17 at 12:29

3

You forgot the Rule of Three and didn't provide a copy-assignment operator. So, when assigning one DataPackage from another, both end up with a pointer to the same buffer, and both try to delete it.

I'd throw that class away and use std::string or std::vector<char> instead.

edited May 23 '17 at 12:29

Community

1
1

answered May 15 '13 at 18:24

Mike Seymour

249,747
28
448
644

std::string is very useful, because the data later contained will contain binary data (and should not necessarily end with '\0'). – michi.0x5d May 15 '13 at 18:49
@michi.0x5d: `string` handles arbitrary binary data just fine, unless you try to convert it to a C-style string; but perhaps `vector` would be a better fit. – Mike Seymour May 15 '13 at 18:51

score 1 · Answer 2 · answered May 15 '13 at 18:22

1

The error is caused by 'pack1 = pack2;' in main.cxx

Implement DataPackage::operator= to fix it.

answered May 15 '13 at 18:22

Mariusz Ceier

33
1
4

score 0 · Accepted Answer · answered May 15 '13 at 18:22

0

You need implement an assignment operator that manages m_data. The automatically generated implementation will just copy the pointer, not copy the pointed-to array. The assignment operator could look like this:

DataPackage& DataPackage::operator=(const DataPackage &other) {
   setData(other.m_data, other.m_length);
   return *this;
}

answered May 15 '13 at 18:22

sth

222,467
53
283
367

Thanks, this solves it (maybe someone will have a similar problem). But maybe it's a [good idea to switch to vector](http://stackoverflow.com/a/16572382/1354246) in this case. – michi.0x5d May 15 '13 at 19:12

score -1 · Answer 4 · answered May 15 '13 at 18:22

Try :

void DataPackage::setData(const char *newdata,size_t newlength){
  char* tmpdata = new char[newlength];
  m_length = newlength;
  memcpy(tmpdata,newdata,m_length);

  if (m_data)
  {
      delete[] m_data;
  }

  m_data = tmpdata;
}

From what i see your copy constructor set m_data to NULL and call setData just after. In setData your are basically doing delete[] NULL;

C : Double free or corruption, can't find error

4 Answers4