views:

95

answers:

4

Hi friends, this might be a bit long so my apologies. consider the following code (i've left some irrelevant parts from it). this code receives a pointer to a struct (BoardP theBoard), x & y coords and a value. the goal is to place the value in a 2D array that is found in the struct. if the coords are out of bounds, i have to increase the size of the table, copy old data to new data and place the value in its place. well this code works the first call but in the second call it crashes and writes:

*** glibc detected *** ./b: double free or corruption (top): 0x092ae138 ***  

i couldn't find an answer to it and i hope you will help.
These are the calls from main()

BoardP p = CreateNewBoard(10,10);
PutBoardSquare(p,10,5,'X');
PutBoardSquare(p,5,10,'O');

Boolean PutBoardSquare(BoardP theBoard, int X, int Y, char val) {

    if (inBounds(X,Y,theBoard->_rows,theBoard->_cols)) {
        theBoard->_board[X * theBoard->_cols + Y] = val;
        return TRUE;
    }
    else {
        int newRows = (X>=theBoard->_rows) ? (2*X) : theBoard->_rows;
        int newCols = (Y>=theBoard->_cols) ? (2*Y) : theBoard->_cols;
        BoardP newBoard = CreateNewBoard(newCols,newRows);  //this creates a new Board with the new dimensions
        if (newBoard == NULL) {
            //ReportError(MEM_OUT);
            return FALSE;
        }
        else {
            copyData(theBoard,newBoard);
            freeBoardArray(&theBoard->_board[0]); //free old array
            theBoard->_board = newBoard->_board;  //old array point to new array
            FreeBoard(newBoard);  //free the temp copy THIS CAUSES THE PROBLEM  
            PutBoardSquare(theBoard,X,Y,val);//recursion, will be in bounds now
            return TRUE;
        }
    }
}

These are the Free functions:

void FreeBoard(BoardP board) {
    if (board != NULL) {
        printf("FREE 1\n");
        //free the board array:
        if (board->_board != NULL) {
            printf("FREE 2\n");
            freeBoardArray(&board->_board[0]);
            printf("FREE 3\n");
        }
        free(board);
    }
}

static void freeBoardArray(char * arrP) {
    free(arrP);   //**PROGRAM CRASH HERE**
}

This is how i create a new board:

BoardP CreateNewBoard(int width, int high) {
    BoardP board = (BoardP) malloc(sizeof(Board));
    if (board != NULL) {
        board->_board = allocateBoardArray(high,width);
        if ( board->_board == NULL) {
            FreeBoard(board);
            //TODO make file ReportError(MEM_OUT);
            return NULL;
        }
        initializeBoard(board,high,width,X_SIGN,SPACE);
        return board;
    }
    else {
        FreeBoard(board);
        //TODO make file ReportError(MEM_OUT);
        return NULL;
    }
}

static char* allocateBoardArray(int row, int col) {
    char* newBoard = (char*) malloc(row * col * sizeof(char));

    if (newBoard == NULL) {
        return NULL;
    }
    return newBoard;
}

this is BoardP:

typedef struct Board* BoardP;
+3  A: 

You have to free memory which you have allocated and no longer want to hold a reference too. from your code i can see the following line.

theBoard->_board = newBoard->_board;

Now you maintain reference to a allocated pointer and then free that same pointer itself.

Example code:

char *foo()
{
char *ref1;
char *ref2;
ref1 = malloc(256);
ref2=ref1;// Holding reference to a pointer in another pointer
strcpy(ref1,"stackoverflow");
printf("%s %s",ref1,ref2); // This prints stackoverflow twice
free(ref1); // This is valid but you can access ref2 or ref1 after this point
return ref2; /// This will cause problems
}
Praveen S
I don't think that is what is happening. newBoard->_board is a separately allocated block from newBoard.
karunski
i understand, but my first problem was to make changes in theBoard what will last after it exits the function. This is the only way i mananged to solve it, however it is wrong as you say. How can i change the data in theBoard that will be valid after the function exits?
rob
Oops, Praveen is right. FreeBoard(newBoard) deletes newBoard->_board. Just do free(newBoard) instead of calling your FreeBoard function.
karunski
it this is my struct: ypedef struct Board { int _rows; int _cols; char *_board;} Board;if i do free(newBoard) it will free the struct but will NOT free the allocated char* _board (the array)?
rob
That is correct. See my answer.
karunski
+1  A: 

Try this:

copyData(theBoard, newBoard);
/* swap the _board pointers */
char * b = theBoard->_board;
theBoard->_board = newBoard->_board;
newBoard->_board = b;
FreeBoard(newBoard);  /* cleanup the temp struct and the old array */
karunski
thanks for the answer.. one last question:if the struct contains also primitive type such as int and char, when freeing the struct does if free them?
rob
Yes. You only free() the things you malloc().
karunski
A: 

This errors says that you are trying to free the memory which is already freed by you. What i am suspecting here is this block of code

if (board != NULL) {
printf("FREE 1\n");
//free the board array:
if (board->_board != NULL) {
    printf("FREE 2\n");
    freeBoardArray(&board->_board[0]);
    printf("FREE 3\n");
}
free(board);

once you are freeing the part of structure freeBoardArray(&board->_board[0]); and then you are freeing the whole structure free(board);, and it looks to me causing the problem.Why you passing the address of the _board pointer?I wrote the code on the same line of code,which causing the problem.

struct a{
    int * next;

}; int main(){

    struct a *aptr = (struct a *)malloc(sizeof(struct a));
    aptr->next=(int *)malloc(5*sizeof(int));
    free(&aptr->next);
    free(aptr);
    return 0;

}

this code will cause the same issue as you shown. Now again try this code after removing '&' from free(&aptr->next);statement.It will work fine. So i think you got a clue where you have to modify.

Anil Vishnoi
karunski
A: 

Running this code under valgrind will tell you exactly on which line you a.) first freed the memory and b.) when you tried to free it again.

It will also tell you if you try and access any addresses which are inside a block that you have freed.

Rob Bradford