segmentation fault with deep copy

Question

I am trying to make a deep copy (for copy on write) of an object but I get a segmentation fault.

I am using a hashtable with linked list.

    class Person
{
public:
    Person(const char * id,int nb)
    {
        this->id=strdup(id);
        this->nb=nb;
        this->init=init;
        this->next=NULL;
    }
    Person(const Person& rhs) :
    nb(rhs.nb),
    init(rhs.init),
    id(strdup(rhs.id)),
    next(rhs.next == NULL ? NULL : new Person(*rhs.next)) {}

    char* strdup(char const* in)
    {
        char* ret = new char[strlen(in)+1];
        strcpy(ret, in);
        return ret;
    }

    int nb,init;
    const char * id;
    Person *next;
};


    Hashtable deepcopy (const Hashtable& rhs)
    {
    num[0]=num[0]-1;
    Person** array=rhs.table;
    Hashtable autre;
    for (int i = 0 ; i < size; ++i)
        if (autre.table[i]!=NULL)
            autre.table[i] = new Person(*array[i]);
    return autre;
    num[0]=1;
}

the attributs of my class Hashtable:

 Person **table;
    int* num;

EDIT: this problem seem to be fixed. What is wrong with my deep copy? I don't understand. I think that my copy constructor is good but I don't understand why I get a seg fault when I run it.

I've removed "cpp" from the question title. First, the language is called C++; "cpp" commonly refers to the C preprocessor. Second, the question is tagged "c++, so there's no need to specify the language in the title. — Keith Thompson, Apr 16 '14 at 22:52
Why in the world are you using dynamic allocation for a single int (or, really, at all)? Where is `size` coming from? Also, `std::copy` — Ed S., Apr 16 '14 at 22:54
I am trying to implement copy on write for my hashtable. The size is defined and equal to 300. I cannot use std. I can use: #include cstdio, cstdlib, cstring, iostream. — bandera, Apr 17 '14 at 07:25
@bandera Please see http://meta.stackexchange.com/questions/40164/should-we-close-fix-my-program-questions — sashoalm, Apr 17 '14 at 08:08

alphashooter · Answer 1 · 2014-04-16T23:20:35.697

This code must be fixed:

for (int i = 0 ; i < size; ++i)
    autre.table[i] = new Person(*array[i]);

table has fixed size, and it's filled with null-pointers. In your loop, you don't check if the element to be copied is a null-pointer, so you derefence it and try to copy the entity which even doesn't exist.

for (int i = 0 ; i < size; ++i) {
    if(array[i] != NULL) {
        autre.table[i] = new Person(*array[i]);
    }
}

PS: It's better to use nullptr instead of NULL.

R Sahu · Answer 2 · 2014-04-17T08:32:52.260

Problems that I see:

Default constructor of Person.
```
Person(const char * id,int nb)
{
  this->id=id;
  this->next=NULL;
}
```
If I use
```
Person foo()
{
  char id[] = "John";
  return Person(id, 0);
}

Person a = foo();
```
Then the stack memory used for holding "John" in foo is now held on to by a, which will lead to undefined behavior.

You need to take ownership of the input string. Use std::string for id instead of char const*.
Copy constructor of Person.

The statement
```
id(rhs.id),
```
will be a problem if you decide to use char const* as type for id. If you switch it to std::string, it won't be a problem.
Copy constructor of HashTable makes a shallow copy of table. This will be a problem if you decide to delete the table in the destructor of HashTable. If you don't delete table in the destructor of HashTable, you have a memory leak.
In deepcopy, you are not checking whether array[i] is NULL before dereferencing it. This has already been pointed out by @alphashooter. Additionally, you are creating a deep copy in a local variable of the function,autre. The deep copy is not visible outside the function unless you return autre from it.

EDIT Since you are not allowed, to use std::string, you will have to allocate memory for the char const* in the default constructor as well as the copy constructor. If your platform has the non-standard function strdup and you are allowed to use it, you can change the default constructor to:

Person(const char * id,int nb)
{
  this->id=strdup(id);
  this->next=NULL;
}

You need to make a similar change to the copy constructor.

If you don't have strdup or you are not allowed to use it, you can define it. It's a very simple function to write.

char* strdup(char const* in)
{
  char* ret = new char[strlen(in)+1];
  strcpy(ret, in);
  return ret;
}

Hello, thank you for your answer. 1) It is a part of an exercise and I cannot use std::string. How can I fix it? 2) I cannot use std string unfortunately. 3) I am trying to implement copy-on-write. I create a shallow copy and make it deep when I want to modify something. 4) Thank you, I now return autre in the function. — bandera, Apr 17 '14 at 07:15
@bandera I updated my answer to explain what you can do if you are not allowed to use `std::string`. — R Sahu, Apr 17 '14 at 07:23
"*Then temporary memory used for holding "John" is held on to by a*" What?! "John" is a constant. The temporary holds a pointer to the constant, but he saves a copy of that pointer, so it doesn't matter if it goes away. — David Schwartz, Apr 17 '14 at 07:33
Thanks, no segmentation fault anymore but now the insertion is not working after a deep copy. I edited my post. — bandera, Apr 17 '14 at 07:54
@DavidSchwartz Perhaps my understanding of what compilers/run time environments do with constants like "John" is flawed. I need a better example of why holding on the input `char const*` will lead to undefined behavior. — R Sahu, Apr 17 '14 at 08:01

segmentation fault with deep copy

2 Answers2