I have the following minimal version of my code (I have set all data fields to public for debugging purposes):

#include <iostream>
#include <vector>
#include <string>
using std::cout; using std::vector; using std::string;

class Population;  // forward declaration

class Person {
    public:
        Person *spouse;
        void marry(Population &p, int i);  // defined after Population class
};

class Population {
    public:
        vector<Person> people;
        void add(Person p) {
            people.push_back(p);
        }
};

void Person::marry(Population &p, int i) {
    *spouse = p.people.at(i);
}

int main() {
    Person john; Person jane;

    Population pop;
    pop.add(john); pop.add(jane);

    john.marry(pop, 1);  // tries to marries Jane, but seg fault
}

It compiles fine, but it gives a segmentation fault during runtime because of the following line:

*spouse = p.people.at(i);

What exactly is giving this seg fault, and how can I get around it?

CodePudding user response：

*spouse = p.people.at(i);

This line of code does the following, in C : 1) Return a reference to the ith object in the p.people vector, and 2) copy the referenced object to another object that this spouse pointer is pointing to.

What exactly is giving this seg fault

Well, a necessary requirement for copying one object, into another object that a pointer is pointing to, is that the pointer, spouse, must be a pointer to a valid, existing object.

There's nothing in the shown code that initializes the spouse pointer to point to some existing object. This is most certainly the reason for your program's crash.

and how can I get around it?

The spouse pointer must be initialized to point to a valid object. However there are several defects in these objects' overall design, as discussed in the comments, so you will likely need to completely redesign your objects' entire hierarchy, and how the various objects relate with each other.

CodePudding user response：

What's the problem?

The problem is that the following statement does not do what you think it does:

*spouse = p.people.at(i);

This statement takes an unitialized pointer spouse, and tries to assign the Person it points to (but it doesn't point to anything valid) by copying the content of p.people.at(i).

Since the spouse pointer is not initialized, this is UB (undefined behavior) and the segfault is one of the many possible symptoms.

How to solve it?

What you probably wanted to do is to assign to the spouse pointer the address of the spouse person:

spouse = &p.people.at(i);

This would work better in the case of your small snippet. But it would not be a reliable solution: if you'd add new people to the population vector, the vector could grow, and in some cases the vector content could be moved to a different place, thus making all the previously obtained pointers to its element invalid (and again, UB if you'd try to access them).

You could instead consider keeping the spouse as an index, or manage the persons in the population in a way that preserves their address, for example creating the persons in a map, or make the vector a vector of addresses to persons that would be copy-created when you do the add()

CodePudding user response：

The issue is caused by dereferencing an uninitialized pointer.

The whole class design needs a bit of ajustment. Otherwise a person can be copied resulting in confusion about who a person is married to.

Given the fact that you want to represent marriage by storing a pointer to the partner means you've got some requirements:

• If the data about a person gets stored in a different memory location, the spouse needs to be informed
• Any move of the memory location should leave only one person valid; otherwise you wouldn't be sure, if the spouse pointer of the spouse points to the intended person.

A solution this problem is implementing move semantics for Person.

class Person {
public:
    Person() = default; // default constructor required by vector; also used for creating unmarried people

    ~Person()
    {
         if (spouse != nullptr)
         {
             // spouse now can no longer access their spouse
             spouse->spouse = nullptr;
         }
    }

    Person(Person&& old)
        : spouse(old.spouse)
    {
        if (spouse != nullptr)
        {
            spouse->spouse = this; // tell spouse the new address of the partner
            old.spouse = nullptr; // prevent destructor of old objects from doing incorrect updates to the spouse
        }
    }

    Person& operator=(Person&& old)
    {
        if (spouse != nullptr)
        {
            spouse->spouse = nullptr;
        }
        spouse = old.spouse;
        if (spouse != nullptr)
        {
            spouse->spouse = this; // tell spouse the new address of the partner
            old.spouse = nullptr; // prevent destructor of old objects from doing incorrect updates to the spouse
        }
        return *this;
    }

    void marry(Person& newSpouse)
    {
        if (spouse != nullptr)
        {
            // still married, so throw an exception or divorce; I'll choose divorce here
            spouse->spouse = nullptr;
        }
        spouse = &newSpouse;
        spouse->spouse = this;
    }

    Person* GetSpouse()
    {
        return spouse;
    }

private:
    // spouse data managed internally; don't provide write access outsize of this class
    Person *spouse {nullptr};

};

Now std::vector<Person> will automatically do updates on a resize of the backing storage, since it uses the move semantics...

std::vector<Person> population;
population.emplace_back();
population.emplace_back();

// note: the reference may be invalidated on a vector resize
auto& john = population[0];
auto& jane = population[1];

john.marry(jane);