0

I am new to C but I am currently working on a C program which I am having an issue with relating to structures and memory allocation.

What I have in my code is a permanent while loop which will break out of when a certain condition is met. Within this while loop it checks the database and has a while loop to loop round the MySQL rows.

When the program first loads, it creates 3 structures, one of the structures also containing a linked list and initialised to be a size of 100.

As my program loops through the MySQL rows it checks whether 100 records have been added, and if so, then does a realloc to increase the size by another 100 rows. When I have looped through all of the MySQL rows it then returns to main while loop (the never ending loop) and NULL's the structures and then initialises them again to be 100 rows and starts all over again.

The problem I am having, is once it does the first complete loop through all of the mysql rows and returns to the main never ending loop and the structures are reinitialised back to be a size of 100, on the 17th row my program segfaults. When I inspect everything in GDB the structure at index 17 comes up saying that the certain elements within the structure are out of bounds.

Below is the definition for the structure I am having the issue with.

typedef struct CallLogSearchDataStruct
{
    char * date;
    char * time;
    char * bParty;
    char * aParty;
    float duration;
    char * cleardownCause;
    struct CallLogSearchOutboundStruct * outboundLegs;
} callLogSearchDataStruct;

Below is how the structure initially get set up when the program first runs

callLogSearchData = calloc(INITIAL_CALL_STRUCT_SIZE,sizeof(callLogSearchDataStruct));
callLogSearch = calloc(INITIAL_CALL_STRUCT_SIZE,sizeof(callLogSearchResultStruct));
switches = calloc(INITIAL_CALL_STRUCT_SIZE, sizeof(switchIDStructure));

INITIAL_CALL_STRUCT_SIZE is equal to 100.

Below is the code for how the reallocateStructures function is called. This reallocates the structures by adding another 100 in size to the original size. 

if (reallocateStructures(&callLogSearch, &callLogSearchData, &switches, &timesStructHasBeenReallocated, currentStructIndexValue, dataRow) == 0)
                    {
                        //Structures have been reallocated so reset the index
                        currentStructIndexValue = 0;
                    }

Below is the actual code for the reallocating of the structures

int reallocateStructures(callLogSearchResultStruct **callLogSearch, callLogSearchDataStruct ** callLogSearchData, 
        switchIDStructure ** switches, int *timesStructHasBeenReallocated, int currentStructIndexValue,
        int dataRow)
{
    int INITIAL_CALL_STRUCT_SIZE = 100;
    int currentSize = 0;
    int newSize = 0;
    int initFromIndex = 0;
    callLogSearchResultStruct * callLogSearchTemp;
    callLogSearchDataStruct * callLogSearchDataTemp;
    switchIDStructure * switchesTemp;


    printf("Current Struct Index Value: %i\n", currentStructIndexValue);

    if (currentStructIndexValue >= INITIAL_CALL_STRUCT_SIZE) {
        printf("REALLOCATING STRUCTURES");
        currentSize = currentStructIndexValue * *timesStructHasBeenReallocated;

        newSize = currentSize + INITIAL_CALL_STRUCT_SIZE;
        *timesStructHasBeenReallocated = *timesStructHasBeenReallocated + 1;

        callLogSearchTemp=  (callLogSearchResultStruct*)realloc(*callLogSearch, (newSize * sizeof(callLogSearchResultStruct)));
        callLogSearchDataTemp = (callLogSearchDataStruct*)realloc(*callLogSearchData, (newSize * sizeof(callLogSearchDataStruct)));
        switchesTemp = (switchIDStructure*)realloc(*switches, (newSize * sizeof(switchIDStructure)));


        /**callLogSearchData = realloc(*callLogSearchData, newSize * sizeof (callLogSearchDataStruct));
        *callLogSearch = realloc(*callLogSearch, newSize * sizeof (callLogSearchResultStruct));
        *switches = realloc(*switches, newSize * sizeof (switchIDStructure));
        */
        for (initFromIndex = currentSize; initFromIndex < newSize; initFromIndex++) {
            callLogSearchDataTemp[initFromIndex].aParty = NULL;
            callLogSearchDataTemp[initFromIndex].bParty = NULL;
            callLogSearchDataTemp[initFromIndex].cleardownCause = NULL;
            callLogSearchDataTemp[initFromIndex].date = NULL;
            callLogSearchDataTemp[initFromIndex].duration = 0;
            callLogSearchDataTemp[initFromIndex].outboundLegs = NULL;
            callLogSearchDataTemp[initFromIndex].time = NULL;

            callLogSearchTemp[initFromIndex].date = NULL;
            callLogSearchTemp[initFromIndex].dRowIndex = dataRow;

            switchesTemp[initFromIndex].switchID = NULL;

            if (initFromIndex == newSize - 1)
            {
                printf("debugging here\n");
            }
        }

        *callLogSearch = callLogSearchTemp;
        *callLogSearchData = callLogSearchDataTemp;
        *switches = switchesTemp;
        return 0;
    }
    else
    {
        return 1;
    }
}

Below is the code where after it has looped through all of the MySQL rows the structures are then reset and re-initialised to be a size of 100. 

//Check if function has looped round already and if so reset all structures
        if (dataRow > -1)
        {
            numberOfTimesEverythingHasReset++;
            callLogSearchData = NULL;
            callLogSearch = NULL;
            switches = NULL;

            callLogSearchData = realloc(callLogSearchData, INITIAL_CALL_STRUCT_SIZE*sizeof(callLogSearchDataStruct));
            callLogSearch = realloc(callLogSearch, INITIAL_CALL_STRUCT_SIZE*sizeof(callLogSearchResultStruct));
            switches = realloc(switches, INITIAL_CALL_STRUCT_SIZE*sizeof(switchIDStructure));

            //initialiseNewStructure(&callLogSearch, &callLogSearchData, &switches);
            //callLogSearchData = (callLogSearchDataStruct*)realloc(callLogSearchData, INITIAL_CALL_STRUCT_SIZE*sizeof(callLogSearchDataStruct));
            //callLogSearch = (callLogSearchResultStruct*)realloc(callLogSearch, INITIAL_CALL_STRUCT_SIZE*sizeof(callLogSearchResultStruct));
            //switches = (switchIDStructure*)realloc(switches, INITIAL_CALL_STRUCT_SIZE*sizeof(switchIDStructure));

            //Initialise all elements within structures
            for (initFromIndex = INITIAL_CALL_STRUCT_SIZE; initFromIndex < INITIAL_CALL_STRUCT_SIZE; initFromIndex++)
            {
                callLogSearchData[initFromIndex].aParty = NULL;
                callLogSearchData[initFromIndex].bParty = NULL;
                callLogSearchData[initFromIndex].cleardownCause = NULL;
                callLogSearchData[initFromIndex].date = NULL;
                callLogSearchData[initFromIndex].duration = 0;
                callLogSearchData[initFromIndex].outboundLegs = NULL;
                callLogSearchData[initFromIndex].time = NULL;

                callLogSearch[initFromIndex].date = NULL;
                callLogSearch[initFromIndex].dRowIndex = dataRow;
            }

            timesStructHasBeenReallocated = 1;
            currentSize = 0;
        }
        currentStructIndexValue = 0;

I'm not sure why when it enters the if statement the first time to reset the structures back to a size of 100, after the 17 MySQL row the memory is then out of bounds so I segfault.

Thanks for any help you can provide.

UPDATE I have fixed the issue with the for loop as per @Rohan answer but I am now getting a slightly different issue.

I am now getting a different area when trying to add data to the structures but I am not sure if the structures are anything to do with it as it seems to be the MySQL array having an out of bounds issue now causing a seg fault.

The line where it is failing is 

callLogSearchData[dataRow].bParty = strdup(rowReport[bPartyColIndex]);

When I inspect rowReport in GDB it seems to have garbage inside it and at the 7th index (bPartyColIndex is at index 9) I start to see the out bounds error.

rowReport is assigned at each time the loop for each mysql row as shown in the code below

sqlLen = asprintf(&sql, "SELECT Tmp.SwitchID, Tmp.CorrelationID, SeizeUTC as Date, "
            "SeizeUTC as Time, Direction, ACMToAns/100 as ACMToAns, Duration/100 as Duration, "
            "CleardownCause, AParty, Tmp.BParty FROM TMP_Log AS Tmp ORDER BY SeizeUTC, "
            "Tmp.SwitchID, Tmp.CorrelationID, Direction, SeizeCSec LIMIT %i, %i",
            index, count);
        SL_DebugAll(DBG_INFO, sql);

        if ((mysql_real_query(HandleDB, sql, sqlLen))) return 1;

        resultReport = mysql_store_result(HandleDB);

        if (mysql_num_rows(resultReport) == 0 || index > atoi(limit))
        {
            SL_DebugAll(DBG_INFO, "Data retrieval for call log complete");
            break;
        }
        else
        {
            numRows = mysql_num_rows(resultReport);
            swID = -1;
            corrID = -1;
            dataRow = -1;
            if (numRows > 0)
            {
                maxTargets = 1;
            }

            audioRow = mysql_fetch_row(audioResult);
            sspSwitchID = atoi(audioRow[switchIDColIndex]);
            sspCorrID = atoi(audioRow[correlationIDColIndex]);
            inbound_counter = 0;

            while (rowReport = mysql_fetch_row(resultReport))
Boardy
  • 35,417
  • 104
  • 256
  • 447
  • Sorry, what are you asking – Boardy Sep 16 '13 at 12:26
  • Probably not your immediate problem, but note that you're not handling possible realloc failures. – Paul R Sep 16 '13 at 12:26
  • Instead of having a variable for the number of times you've reallocated the structures, why not have one with the current index, and one with the size. The "current index" goes ever upward, and is the index of the last used entry in the allocated "arrays". The size is the number of entries allocated, and starts out with `INITIAL_CALL_STRUCT_SIZE`, and it too grows ever upward. If you increase the "current index" and it's equal to the "current size", then it's time to reallocate and increase "current size. – Some programmer dude Sep 16 '13 at 12:28
  • 1
    Also, if `currentStructIndexValue` is the index of the last entry in the arrays, then resetting it to zero will cause you to overwrite the old entries. – Some programmer dude Sep 16 '13 at 12:30
  • @JoachimPileborg thanks I don't the calculation for the new size is the problem, although your suggestion is probably a cleaner way than what I am using. There's a separate index that I am using called dataRow for adding elements to the structures. The ``currentStructIndexValue`` will just go from 0 to 100, at 100 the realloc is done and if successful realloc is returned the variable is reset to 0 and starts again – Boardy Sep 16 '13 at 12:40
  • @PaulR thanks, I am aware I'm not checking if the malloc,realloc and callocs are failing, at the moment I don't believe they are but I am intending to put them in. I'm just trying to keep the code as minimal as possible to help with debugging to get it basically working – Boardy Sep 16 '13 at 12:41
  • If under linux try to use `valgrind`. It'll point you to the line where the invalid read occurs. – Dariusz Sep 16 '13 at 12:43
  • @Dariusz I have looked at Valgrind and gone through the tutorials but some of the messages don't make a lot of sense as to what the problem is to me. For example it sometimes says conditional jump based on uninitialised values even though they are initialised – Boardy Sep 16 '13 at 12:51
  • @Boardy valgrind is never wrong. Initialize your variables to 0 or NULL to get rid of this message. Or just grep -a 10 the result for invalid read/invalid write. – Dariusz Sep 16 '13 at 12:54
  • I have initialised them to 0 or NULL. I do get the invalid read write messages as well. Again I'm not entirely sure what they mean. Based on other code I've looked at it seems to be correct – Boardy Sep 16 '13 at 12:58

2 Answers2

0

In last code section of yours you have

if (dataRow > -1)
    {
        numberOfTimesEverythingHasReset++;
        callLogSearchData = NULL;
        callLogSearch = NULL;
        switches = NULL;

        callLogSearchData = realloc(callLogSearchData, 
                              INITIAL_CALL_STRUCT_SIZE*sizeof(callLogSearchDataStruct));
        callLogSearch = realloc(callLogSearch, 
                            INITIAL_CALL_STRUCT_SIZE*sizeof(callLogSearchResultStruct));
        switches = realloc(switches, 
                      INITIAL_CALL_STRUCT_SIZE*sizeof(switchIDStructure));


        //Initialise all elements within structures
        for (initFromIndex = INITIAL_CALL_STRUCT_SIZE; 
             initFromIndex < INITIAL_CALL_STRUCT_SIZE; 
             initFromIndex++)
        {
           ....

You are setting callLogSearchData to NULL so reallocating is not relevant.

And you are allocating for INITIAL_CALL_STRUCT_SIZE number of elements, but your for loop counter starts from INITIAL_CALL_STRUCT_SIZE so effectively for loop does not execute.

Rohan
  • 52,392
  • 12
  • 90
  • 87
  • Oh yea seems obvious now. So to reset the structures to be empty but only have 100 items again, what would be the best method, not null the structures and just perform a calloc, or is the realloc fine, I just don't need to NULL them first – Boardy Sep 16 '13 at 13:32
  • @Boardy, I think start `for loop` from `0` to `INITIAL_CALL_STRUCT_SIZE`. – Rohan Sep 16 '13 at 13:35
  • Yea I understood that bit but you also said I'm setting callLogSearchData to NULL so reallocating is not relevant – Boardy Sep 16 '13 at 13:45
  • @Boardy, If you are reallocating don't need to do anything else except, resetting values appropriately to `0 or NULL` and setting your variables tracking the length to new value. – Rohan Sep 16 '13 at 13:49
0

I've managed to find out what the issue, its a pretty basic mistake unfortunately.

Because of where the code was crashing, I thought it had to be something related to the way I was re-initialised the structure, maybe because of the way I was using pointers.

However, this was not the case instead, instead, somewhere else in the code I was adding some data to one of the structures using an index called fileRow. Each time a value was added to the structure I incremented fileRow however I forgot to reset back to 0 when I realloced the structures, so when I realloced the structures to be of size 100, I was inserting into the structure of where fileRow was which was set to 1300 so each time I looped round I smashed up the memory until it segfaulted.

Thanks for your help and suggestions.

Boardy
  • 35,417
  • 104
  • 256
  • 447