My code to delete a structure entry does not delete, and I cannot figure out why

Question

I am currently trying to make a chess game. However, I found a bug where if I try to delete a piece (when it is killed) the program freezes and then quits.

The problem seems to be in the DeletePiece function. I was able to figure this out by moving printf's through my code until I could pinpoint where it's breaking. As soon as I try to free something, it freezes.

typedef struct {
        char *color;
        char *piece;
} PIECE;

PIECE *createPiece(char *color,char *piece) {

    PIECE *p=malloc(sizeof(PIECE));

    if(p==NULL) {
        printf("The memory allocation failed");
        return NULL;
    }
    else {
        p->color=malloc(2*sizeof(unsigned char));
        p->color=color;
        printf("piece is: %c \n",p->color[0]);
        p->piece=malloc(2*sizeof(unsigned char));
        p->piece=piece;
    }
}

PIECE *deletePiece(PIECE *p) {
    if(p!=NULL) {
        assert(p);
        assert(p->piece);
        assert(p->color);

        free(p->color);
        free(p->piece);
        p->color=NULL;
        p->piece=NULL;
        free(p);
    }
}

Answer 1

p->color=malloc(2*sizeof(unsigned char));
p->color=color;

This is not doing what you think it's doing. The first line allocates some space in memory and stores its address in p->color; the second line overwrites p->color with another address. The allocated memory is now lost since you lost its address.

You probably wanted to copy the contents of color into the space allocated to p->color, and you don't do that with an simple pointer assignment. Also, why allocate memory dynamically if you know how much you need everytime (2*sizeof(unsigned char))? Just use arrays in your structure:

typedef struct {
    char color[2];
    char piece[2];
} PIECE;

Actually, the same thing could be said about dynamically allocating pieces. There are only 32 pieces in chess. No more, no less. Just use an array of pieces:

PIECE pieces[32];

Another point I don't get, but I can't really tell without seeing your full code, is that you use 2 characters for each color and piece. I'm relatively sure you only need one character, or better, one enumeration value, to store a color or a piece. Which eliminates the need for arrays in the first place.