shared_ptr assignment in recursive function causing Segmentation Fault

Question

Apologies in advance for posting so much code...

I'm working on building a simulation of a dominoes-like game called Chickenfoot in which players draw "Bones" from a boneyard into their hands and then play dominoes on a field.

This is the first program in which I've tried using smart pointers and I've run into an issue that I cannot seem to pinpoint the cause. Occasionally running this program gives me a segmentation fault. A gdb stack trace can be seen below.

Strange shared_ptr behaviour this answer suggests that this could have something to do with this being a recursive function.

What am I doing wrong here? Also, if I am misusing any of these shared_ptr instances or could improve the implementation, any advice would be really appreciated - Thanks!

ChickenFoot.cpp

#include <iostream>
#include <cstdlib>
#include <ctime>
#include "Game.h"

const int NUM_OF_PLAYERS = 4;

int main(int argc, char** argv) {
    std::srand(std::time(0));

    Game* chickenfoot = new Game(NUM_OF_PLAYERS);
    chickenfoot->start(DOMINOES_SET_SIZE);

    delete chickenfoot;

    return 0;
}

Game.h

#include <memory>
#include <vector>
#include "Boneyard.h"
#include "Player.h"
#include "Field.h"

const int INITIAL_HAND_SIZE = 7;
static const int DOMINOES_SET_SIZE = 9;

const bool DEBUG = false;

class Game {
private:
    std::vector< std::shared_ptr<Player> > players;
    std::shared_ptr<Boneyard> boneyard;
    bool played_rounds[DOMINOES_SET_SIZE]; // This will keep track of which double rounds have already been played

    int getHighestUnplayedRound(bool* played);
    int getNextHighestUnplayedRound(bool* played, int round);
public:
    Game(int num_of_players);
    void start(int highest_double);
};

Game.cpp

#include "Game.h"
#include <iostream>

Game::Game(int num_of_players)
{
    boneyard = std::make_shared<Boneyard>();
    for (int i = 0; i < num_of_players; i++) {
        players.emplace_back(std::make_shared<Player>(i));
    }
    for (int i = 0; i <= DOMINOES_SET_SIZE; i++) {
        played_rounds[i] = false;
    }
}
void Game::start(int highest_double)
{
    if (highest_double < 0) {
        return;
    } else {
        boneyard->initialize();
        for (int i = 0; i < INITIAL_HAND_SIZE; i++) {
            for (std::vector< std::shared_ptr<Player> >::iterator j = players.begin(); j != players.end(); j++) {
                (*j)->draw(boneyard);
            }
        }
        for (std::vector< std::shared_ptr<Player> >::iterator i = players.begin(); i != players.end(); i++) {
            if ((*i)->hasDouble(highest_double)) {
                std::shared_ptr<Bone> hd_bone = (*i)->getDouble(highest_double);
                // Do something here to actually play the game...
                played_rounds[highest_double] = true;
                break;
            }
        }
    }
    for (std::vector< std::shared_ptr<Player> >::iterator i = players.begin(); i != players.end(); i++) {
        (*i)->discardAll();
    }
    if (played_rounds[highest_double]) {
        start(getHighestUnplayedRound(played_rounds));
    } else {
        start(getNextHighestUnplayedRound(played_rounds, highest_double));
    }
}

Player.h

#include "Bone.h"
#include "Boneyard.h"
#include <vector>
#include <memory>

class Player {
private:
    int id;
    std::vector< std::shared_ptr<Bone> > hand;
    struct isDouble {
        int m_value;
        isDouble(int value) : m_value(value) {}
        bool operator()(const std::shared_ptr<Bone> b) const {
            return (b->getLeft() == m_value && b->isDouble());
        }
    };

public:
    Player(int id);
    void draw(std::shared_ptr<Boneyard> yard);
    std::shared_ptr<Bone> getDouble(int number);
    bool hasDouble(int number);
    void discardAll();
};

Player.cpp

#include <iostream>
#include <algorithm>
#include "Player.h"
...
std::shared_ptr<Bone> Player::getDouble(int number)
{
    auto result = std::find_if(hand.begin(), hand.end(), isDouble(number));
    if (result != hand.end()) {
        hand.erase(std::remove_if(hand.begin(), hand.end(), isDouble(number)), hand.end());
        return *result;
    }
    return nullptr;
}
bool Player::hasDouble(int number)
{
    auto result = std::find_if(hand.begin(), hand.end(), isDouble(number));
    return (result != hand.end()) ? true : false;
}
void Player::discardAll()
{
    hand.clear();
}

trace:

(gdb) backtrace
#0  0x0000000000401a26 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x622d10) at /usr/include/c++/5/bits/shared_ptr_base.h:150
#1  0x0000000000401505 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=0x7fffffffd548, __in_chrg=<optimized out>) at /usr/include/c++/5/bits/shared_ptr_base.h:659
#2  0x0000000000401368 in std::__shared_ptr<Bone, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x7fffffffd540, __in_chrg=<optimized out>) at /usr/include/c++/5/bits/shared_ptr_base.h:925
#3  0x0000000000401384 in std::shared_ptr<Bone>::~shared_ptr (this=0x7fffffffd540, __in_chrg=<optimized out>) at /usr/include/c++/5/bits/shared_ptr.h:93
#4  0x0000000000405ad4 in Game::start (this=0x622030, highest_double=6) at Game.cpp:28
#5  0x0000000000405b8b in Game::start (this=0x622030, highest_double=7) at Game.cpp:39
#6  0x0000000000405b8b in Game::start (this=0x622030, highest_double=9) at Game.cpp:39
#7  0x0000000000405b8b in Game::start (this=0x622030, highest_double=8) at Game.cpp:39
#8  0x0000000000405bb7 in Game::start (this=0x622030, highest_double=9) at Game.cpp:41
#9  0x0000000000405b8b in Game::start (this=0x622030, highest_double=4) at Game.cpp:39
#10 0x0000000000405bb7 in Game::start (this=0x622030, highest_double=5) at Game.cpp:41
#11 0x0000000000405bb7 in Game::start (this=0x622030, highest_double=6) at Game.cpp:41
#12 0x0000000000405bb7 in Game::start (this=0x622030, highest_double=7) at Game.cpp:41
#13 0x0000000000405bb7 in Game::start (this=0x622030, highest_double=8) at Game.cpp:41
#14 0x0000000000405bb7 in Game::start (this=0x622030, highest_double=9) at Game.cpp:41
#15 0x0000000000408360 in main (argc=1, argv=0x7fffffffdaf8) at ChickenFoot.cpp:14

Answer 1

The problem is here...

std::shared_ptr<Bone> Player::getDouble(int number)
{
    auto result = std::find_if(hand.begin(), hand.end(), isDouble(number));
    if (result != hand.end()) {
        hand.erase(std::remove_if(hand.begin(), hand.end(), isDouble(number)), hand.end());
        return *result;
    }
    return nullptr;
}

You're erasing the value before returning it. You can't do that. Once you call hand.erase(), result (which is an iterator) is invalidated, and *result is garbage.

The function is pretty confusing in general, but I think you're shooting for something like this...

std::shared_ptr<Bone> Player::getDouble(int number)
{
    auto result_iter = std::find_if(hand.begin(), hand.end(), isDouble(number));

    if (result_iter != hand.end()) {
        // Saving the shared_ptr stops it from being released when we erase the iterator
        std::shared_ptr<Bone> result = *result_iter;

        // Remove the bone from hand
        hand.erase(result_iter);

        return result;
    }

    return nullptr;
}

---

Let me just add how I found this, because it boils down to reading the stacktrace.

The recursive calls to start are suspicious, but harmless. This isn't a stack overflow error, so you're cool there.

The top 4 lines indicate that you're having an error in the destructor of a shared_ptr (meaning its data is corrupt somehow) and the line is Game.cpp:28 which is the line immediately after std::shared_ptr<Bone> hd_bone = (*i)->getDouble(highest_double);.

This is more or less a guarantee that your error is in getDouble which is a small enough function you can just focus on it to find the error.

The error here is unrelated to Strange shared_ptr behaviour. In that case, the shared_ptr destructor call was happening recursively. That's not happening here, where the shared_ptr destructor is only happening once. This is a simple matter of you having a shared_ptr with corrupt data.