c++ a heap has been corrupted error when running the program

Question

I have the following program and every time I'm running it sometimes (most of the times) I'm getting heap corruption error

I can't put my finger where its happenning since every run it pops in different place on my program.

can someone put some light on it please?

P.S heap corruption also pop when I'm trying to free p

Thanks in advance

#define _CRT_SECURE_NO_WARNINGS
#include "stdafx.h"
#include<iostream>
#include<string.h>

using namespace std;
const int MAX_OF_PLAYERS = 10;
const int SIZE = 100;

struct player_t {
    char *name;
    int numOfShirt;
};
struct team_t {
    char *nameOfTeam;
    int maxOfPlayers;
    int numOfPlayers;
    player_t *players;

};

void readPlayer(player_t *player);
void initTeam(team_t *team);
void addPlayer(team_t *team);
void printTeam(team_t *team);
void freeAll(team_t *team);
player_t** getAllPlayersStartWithA(team_t *team);
void printAteam(player_t **p);

int main()
{
    team_t t;
    player_t **p;
    initTeam(&t);
    addPlayer(&t);
    addPlayer(&t);
    printTeam(&t);
    p = getAllPlayersStartWithA(&t);
    if (p[0] != NULL)
        printAteam(p);
    system("pause");
    freeAll(&t);
    //delete[] p;
}

void readPlayer(player_t *player)
{
    char name[SIZE];
    cout << " please enter the name of the player " << endl;
    cin >> name;
    cout << " please enter the num of the shirt " << endl;
    cin >> player->numOfShirt;
    int size = strlen(name);
    char *res = new char[size + 2];
    strcpy(res, name);
    player->name = res;
}
void initTeam(team_t *team)
{

    char name[SIZE];

    // get the team name
    cout << " please enter your team name" << endl;
    cin >> name;
    // get the name length
    int size = strlen(name);
    // allocate new array with length size
    team->nameOfTeam = new char[size + 1];
    // copy the string to the new array
    strcpy(team->nameOfTeam, name);

    // get the number of max players
    cout << "please enter the number of the max players on your team" << endl;
    cin >> team->maxOfPlayers;

    // create new players array
    player_t *players = new player_t[team->maxOfPlayers];
    // initial the players array
    for (int i = 0; i < team->maxOfPlayers; i++)
    {
        players[i] = { 0 };
    }
    //bind the array to team
    team->players = players;

    // set current players to 0
    team->numOfPlayers = 0;

}
void addPlayer(team_t *team)
{

    for (int i = 0; i < team->maxOfPlayers; i++)
    {
        if (team->players[i].name == NULL)
        {
            readPlayer(team->players + i);
            break;
        }
    }

}
void printTeam(team_t *team)
{

    cout << "Team name: ";
    cout << team->nameOfTeam << endl;

    cout << "Max Number of players in team: ";
    cout << team->maxOfPlayers << endl;

    cout << "Current number of players in team: ";
    cout << team->numOfPlayers << endl;

    cout << "Team Players:" << endl;
    for (int i = 0; i < team->maxOfPlayers; i++)
    {
        if (team->players[i].name)
        {
            cout << "Player name: ";
            cout << team->players[i].name;
            cout << ", ";
            cout << "Player shirt: ";
            cout << team->players[i].numOfShirt << endl;
        }
    }
    cout << endl;
}
void freeAll(team_t *team)
{
    for (int i = 0; i < team->maxOfPlayers; i++)
    {
        if ((team->players + i)->name != NULL)
            delete[](team->players + i)->name;
    }

    delete[] team->players;
}
player_t** getAllPlayersStartWithA(team_t *team)
{
    int sum = 0, position = 0;
    for (int i = 0; team->players[i].name != NULL; i++)
    {
        if (team->players[i].name[0] == 'a' || team->players[i].name[0] == 'A')
        {
            sum++;
        }
    }

    player_t **p = new player_t*[sum + 1];

    for (int i = 0; i < team->maxOfPlayers; i++)
    {
        p[i] = NULL;
    }

    for (int i = 0; team->players[i].name != NULL; i++)
    {
        if (team->players[i].name[0] == 'a')
        {
            p[position++] = team->players + i;
        }
    }
    return p;
}
void printAteam(player_t **p)
{
    cout << "Players start with 'A': " << endl;
    for (int i = 0; p[i] != NULL; i++)
    {
        cout << "Player name: ";
        cout << (p[i]->name);
        cout << ", ";
        cout << "Player shirt: ";
        cout << (p[i]->numOfShirt) << endl;
    }
}

Answer 1

I have not reviewed the whole code, but there are tools available that can help you in this situation that track the usage of memory and indicate if something went wrong. One example is valgrind that is at least available for Linux environments. Anyway, this tool allowed me to find at least one bug in your code as follows.

1. Compile with debug information. If you are using gcc, use the -g command line flag e.g.

   g++ foo.cpp -g -o foo -std=gnu++11
   

2. Run with valgrind

   valgrind ./foo
   

3. Look at the output

   ==6423== Memcheck, a memory error detector
   ==6423== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
   ==6423== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
   ==6423== Command: ./foo
   ==6423== 
    please enter your team name
   sdfads
   please enter the number of the max players on your team
   3
    please enter the name of the player 
   efwf
    please enter the num of the shirt 
   5
    please enter the name of the player 
   dsfdsa
    please enter the num of the shirt 
   3
   Team name: sdfads
   Max Number of players in team: 3
   Current number of players in team: 0
   Team Players:
   Player name: efwf, Player shirt: 5
   Player name: dsfdsa, Player shirt: 3
   
   ==6423== Invalid write of size 8
   ==6423==    at 0x4011FF: getAllPlayersStartWithA(team_t*) (foo.cpp:155)
   ==6423==    by 0x400C08: main (foo.cpp:38)
   ==6423==  Address 0x5ab6668 is 0 bytes after a block of size 8 alloc'd
   ==6423==    at 0x4C2E80F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   ==6423==    by 0x4011CC: getAllPlayersStartWithA(team_t*) (foo.cpp:151)
   ==6423==    by 0x400C08: main (foo.cpp:38)
   ==6423== 
   ==6423== 
   ==6423== HEAP SUMMARY:
   ==6423==     in use at exit: 72,719 bytes in 3 blocks
   ==6423==   total heap usage: 8 allocs, 5 frees, 74,829 bytes allocated
   ==6423== 
   ==6423== LEAK SUMMARY:
   ==6423==    definitely lost: 15 bytes in 2 blocks
   ==6423==    indirectly lost: 0 bytes in 0 blocks
   ==6423==      possibly lost: 0 bytes in 0 blocks
   ==6423==    still reachable: 72,704 bytes in 1 blocks
   ==6423==         suppressed: 0 bytes in 0 blocks
   ==6423== Rerun with --leak-check=full to see details of leaked memory
   ==6423== 
   ==6423== For counts of detected and suppressed errors, rerun with: -v
   ==6423== ERROR SUMMARY: 2 errors from 1 contexts (suppressed: 0 from 0)
   

4. Apparently you have a problem in line 155 according to this output

   ==6423== Invalid write of size 8
   ==6423==    at 0x4011FF: getAllPlayersStartWithA(team_t*) (foo.cpp:155)
   

   And if we look closer, we see the following:

   player_t **p = new player_t*[sum + 1];
   for (int i = 0; i < team->maxOfPlayers; i++)
   {
        p[i] = NULL;
   }
   

   You create an array of size sum+1, but iterate over it up to team->maxOfPlayers that might or might not be the same. This means you write to some memory outside of the array that you want to modify and therefore you write somewhere in the heap where you should not (leading to a heap corruption).

This is at least one problem. Repeat 1.-4. until valgrind has nothing else to complain about.

Answer 2

You need to take more care with how you're accessing arrays and keeping track of the sizes of them.

As a comment already pointed out, for (int i = 0; team->players[i].name != NULL; i++) is asking for trouble. You should ensure that the loop is constrained by something, either the total number of entries in the players array (presumably maxOfPlayers) or the current number of valid players which seems like it should be numOfPlayers. However your code never actually increments numOfPlayers when adding new entries.

The following code from getAllPlayersStartWithA is also a problem:

player_t **p = new player_t*[sum + 1];

for (int i = 0; i < team->maxOfPlayers; i++)
{
    p[i] = NULL;
}

The sum + 1 can easily be much less than maxOfPlayers. In that case the for loop will overwrite memory beyond the end of the array. This could very well be the cause of your current heap corruption error.