error pushing structure objects with string members into a std::vector

Question

I'm a C#, C programmer learning C++ and I'm having a bit of a trouble. I'm trying to push an object of the struct type 'Person' into a vector but the string values which are members of the Person type are not copied. Also the code exits with an error message - posted at the bottom:

#include <iostream>
#include <vector>
#include <string>
#include <stdio.h>
#include <string.h>

using namespace std;

typedef struct Person {
    string Name;
    string Lastname;
    int Age;
} Person;

void CreatePerson(Person* in_person, string in_name, string in_last,
    int in_age) 
{
    Person t_person;
    t_person.Name = in_name;
    t_person.Lastname = in_last;
    t_person.Age = in_age;

    memcpy(in_person, &t_person, sizeof(t_person));
}

int main(int argc, char *argv[]) 
{
    vector<Person> people;
    Person t_ppl;

    CreatePerson(&t_ppl, "Zareh", "Petros", 13);
    people.push_back(t_ppl);

    CreatePerson(&t_ppl, "Tina", "Yarroos", 26);
    people.push_back(t_ppl);

    int ii;
    for(ii=0; ii < people.size() ; ii++) {
        cout << "Element - " << ii << endl;
        cout << "name:" << people[ii].Name << endl;
        cout << "lastname:" << people[ii].Lastname << endl;
        cout << "age:" << people[ii].Age << endl;
    }

    return 0;
}

And here is the error message:

*** glibc detected *** ./a.out: double free or corruption (fasttop): 0x09d48048 ***
======= Backtrace: =========
/lib/i386-linux-gnu/libc.so.6(+0x75ee2)[0xb74e8ee2]
/usr/lib/i386-linux-gnu/libstdc++.so.6(_ZdlPv+0x1f)[0xb76e551f]
/usr/lib/i386-linux-gnu/libstdc++.so.6(_ZNSs4_Rep10_M_destroyERKSaIcE+0x1b)[0xb76cc99b]
/usr/lib/i386-linux-gnu/libstdc++.so.6(+0x909dc)[0xb76cc9dc]
/usr/lib/i386-linux-gnu/libstdc++.so.6(_ZNSsD1Ev+0x2e)[0xb76cca4e]

Answer 1

std::string is a class and should not be copied by a memcpy operation. It may hold instance-specific data, which will get scrambled if held by two different instances (and probably that's the cause of your problems).

Imagine, that std::string is something like:

class string
{
private:
    char * data;
    int dataLength;
};

If you memcpy one string to another, both data and dataLength are copied to another place (lately being treated as a normal string instance). However, when the destructor is called on these strings (when they run out of scope), they will attempt to free the data held in data field. First string (which is an actual owner of this pointer) will free memory pointed to by this pointer. But then another destructor of your copied string will run and will try to free this memory again, what is not permitted.

Note, that it's exactly, what your system is reporting: double freeing of memory.

Your code is very C-style. In C++ one would create a class with constructor rather than a function, which fills in a struct. I would write your code in the following way:

#include <iostream>
#include <vector>
#include <string>
#include <stdio.h>
#include <string.h>

using namespace std;

struct Person 
{
public:
    string Name;
    string Lastname;
    int Age;

    Person(string newName, string newLastname, int newAge)
        : Name(newName), Lastname(newLastname), Age(newAge)
    {

    }
};

int main(int argc, char *argv[]) 
{
    vector<Person> people;

    Person person1("Zareh", "Petros", 13);
    people.push_back(person1);

    Person person2("Tina", "Yarros", 26);
    people.push_back(person2);

    for(unsigned int i=0; i < people.size() ; i++) 
    {
        cout << "Element - " << i << endl;
        cout << "name:" << people[i].Name << endl;
        cout << "lastname:" << people[i].Lastname << endl;
        cout << "age:" << people[i].Age << endl;
    }

    getchar();

    return 0;
}

The role of your creation method takes the class constructor. It fills in fields of the class properly. Also, C++ provides default copy-constructor and assignment operator, which will handle assigning one person to another properly.

---

Some side notes about your code style.

• In C++ avoid using memcpy. If you have a need of using it, you probably should consider creating proper copy constructor, std::copy or simply making an assignment (what would work perfectly in your situation). memcpy should be used only when copying raw chunks of memory.

• typedef is no longer required for structs in C++. Instead of writing:

  typedef struct Name { ... } Name;
  

  You can simply write:

  struct Name { ... };
  

Answer 2

You've already been told you shouldn't be using memcpy in this situation, so I won't bother repeating that.

The problem with your CreatePerson goes well beyond using memcpy, and just changing to std::copy isn't really going to make it right.

Instead of a free function to create a person, you should almost certainly write that functionality as a constructor instead:

struct Person {
    string Name;
    string Lastname;
    int Age;

    Person(string Name, string Last, int Age) 
       : Name(Name), LastName(Last), Age(Age) 
    {}
};

With this, we can create Person objects much more cleanly:

std:::vector<Person> people;

people.push_back(Person("Zarah", "Petros", 13));
people.push_back(Person("Tina", "Yarroos", 26));

I'd also write an inserter that's responsible for displaying a Person in the proper format:

std::ostream &operator<<(std::ostream &os, Person const &p) { 
    return os << "Name: " << p.Name < "\n"
              << "Last: " << p.LastName << "\n"
              << "Age:  " << p.Age << "\n";
}

With this, your mainstream of your code can insert complete Person objects into a stream without paying attention to the internal details of what a Person contains or how it should display:

for (int i=0; i<people.size(); i++)
    std::cout << people[i] << "\n";

If you want to be a little more ambitious, you can use a standard algorithm instead:

std:copy(people.begin(), people.end(), 
         std::ostream_iterator<Person>(std::cout, "\n"));

Or, if you're using a relatively new compiler, you can use a range-based for loop:

for (auto &p : people)
    std::cout << p << "\n";

Putting all that together, your complete program ends up something like this:

#include <string>
#include <iostream>
#include <vector>

using std::string;

struct Person {
    string Name;
    string LastName;
    int Age;

    Person(string Name, string Last, int Age) 
       : Name(Name), LastName(Last), Age(Age) 
    {}
};

std::ostream &operator<<(std::ostream &os, Person const &p) { 
    return os << "Name: " << p.Name << "\n"
              << "Last: " << p.LastName << "\n"
              << "Age:  " << p.Age << "\n";
}

int main(){ 
    std::vector<Person> people;

    people.push_back(Person("Zarah", "Petros", 13));
    people.push_back(Person("Tina", "Yarroos", 26));

    for (auto &p : people)
        std::cout << p << "\n";
    return 0;
}

Answer 3

Do not use memcpy() on non-POD types. It does not call copy-constructors.

Use std::copy() instead.

In this case, it is easier to do an assignment. Replace:

memcpy(in_person, &t_person, sizeof(t_person));

with

*in_person = t_person;