malloc: *** error for object: pointer being freed was not allocated

Question

I keep getting this error when I run my code. I'm not sure why I keep getting this error because I call delete on two objects which I have allocated using the new operator.

I have tried running it in Xcode and I get a thread 1: signal SIGABRT error.

PokerV2.cpp

int main() {    
    bool game_flag = true;

    Poker_Game game1;

    game1.initialise_game();

    game1.game_run();

    while (game_flag) { 
        char input = 'a';
        game_flag = false;
        game1.game_run(game1.get_current_players());
        std::cout << "Press 'y' to continue";
        std::cin >> input;
        if (input == 'y') {
            game_flag = true;
        }
    }
}

poker_game_class.hpp

void Poker_Game::game_run() {
    int lowest = 10;
    int num = 0;
    // Create the deck and table dynamically
    Deck *deck = new Deck();
    Table *table = new Table(_player_num);

    deck->deal_cards(table->get_all_players(), _player_num, *table);

    for (int i = 0; i < 4; i++) {
        table->set_game_flag(i);
        table->set_highest_bet(0);

        for (int z = 0; z < table->get_player_num(); z++) {
            table->players_turn(z);
        }
    }

    for (int i = 0; i < table->get_player_num(); i++) {
        if (table->get_player(i).calculate_score(*table) < lowest) {
            num = i;
        }
    }

    std::cout << "The winner is player: " << num << "\n";
    std::cout << "The winner has won: £" << table->get_pot() << "\n";

    //Add total pot to players cash

    float current_balance = table->get_player(num).get_balance();
    float balance_after_win = current_balance + table->get_pot();

    table->get_player(num).set_balance(balance_after_win);

    std::cout << "Winners balance: £" << table->get_player(num).get_balance();

    this->Current_players = table->get_all_players();

    delete deck;
    delete table;
}

The error occurs in the game_run function just when the deck and table are deleted.

The output from the terminal

Below is the linked GitHub repository

https://github.com/mbh1620/PokerV2/tree/master/PokerV2

In this particular code there seems to be no need to allocate these objects on the heap at all. — Maxim Egorushkin
@MaximEgorushkin If delete deck doesn't work after deck = new Deck(), then allocating the object on the stack isn't going to fix whatever's actually wrong. — jamesdlin
In the main loop you pass a function call into the 'game1.run()' call . . . I'm just wondering what that is really doing as the method does not accept any parameters. — Paul Gibson

Martin York Martin York · Accepted Answer · 2019-07-08T17:00:20

You have a problem here:

Player * Table::get_all_players()
{
    return Players;
}

You are returning a pointer (and ownership) to Poker_Game when it calls this function:

    // One place you call it from.
    set_current_players(table -> get_all_players());

The problem is that two objects are now claiming ownership of the Players and both call delete [] on the pointer they have.

Poker_Game::~Poker_Game()
{
    delete[] Current_players;
}
Table::~Table()
{
    delete[] Players;
}

Not saying this is your only error.

You have basically fallen in to the trap of bad ownership semantics within your application. In C++ we resolved this problem (unlike C) by using specific type and language constructs to explicitly mark ownership. This way we explicitly know who owns an object and thus who is responsible for deleting them.

Your problems can easily be solved by doing a couple of things.

Don't dynamically allocate arrays.
Use std::vector<> to manage the allocation for you.
Don't dynamically allocate objects whose lifespan does not live longer than the function.
Simply use a local variable and let the scope rules handle it.
If you must dynamically allocate an object use a smart pointer.
std::unique_ptr and std::shared_ptr manage the memory for allocated objects very well.
If you pass an object to another object (and are not passing ownership) then pass by reference (rather than pointer).
This tells the other class they don't own the object and thus should not delete it.

A simple rule to live by is "Separation of Concerns".

A class should either be business logic or it should be resource management. So your class should either handle the logic of poker or they should handle the logic of managing the memory you allocate (not both). So any class that is handling a poker thing should not call new/delete a class that handles new/delete should not have poker logic in it.

Now the standard libraries provide a lot of resource management code so you don't need to, you can simply use the existing code and concentrate on your poker logic. If after the game is working you decide the standard resource management is not efficient enough you can concentrate on upgrading that as a separate task.

malloc: *** error for object: pointer being freed was not allocated

1 Answers