C: free(): invalid pointer Aborted (core dumped) error

Question

I am not sure why I am getting this error, I am running a steganography program which takes in a PPM file and allows a user to encode a message into the PPM file and it I tried to debug in valgrind and the error occurs:

#include <stdio.h>
#include <stdlib.h>
#include <stdbool.h>
#include <string.h>

struct PPM {
    char *comments;
    char *ppmname;
    unsigned int width, height, max;
    unsigned int size;
    struct Pixel**pixels;

}ppm;

struct Pixel {
    int r, g, b;
}Pixel;

static int *decimalToBinary(const char *message, unsigned int msgSize);
struct PPM * getPPM(FILE * f);
struct PPM *encode(struct PPM *im, char *message, unsigned int mSize, unsigned int secret);
void showPPM(struct PPM * im);

int main(int argc, char *argv[]){
    FILE * fin = fopen("home-cat.ppm", "r");

    if(fin == NULL) {
        perror("Cannot open file");
        exit(1);
    }

    struct PPM *p = getPPM(fin);
    struct PPM *g = encode(p, "test", 5, 2);
    showPPM(g);

    free(p->comments);
    free(p);



    return 0;
}

struct PPM * getPPM(FILE * f) {

    ppm.comments = (char *)calloc(70,sizeof(char));
    ppm.ppmname = (char *)calloc(3,sizeof(char));
    fscanf(f, "%s", ppm.ppmname);

    fgetc(f);
    fgets(ppm.comments, 70, f);
    fscanf(f, "%d %d", &ppm.width, &ppm.height);

    fscanf(f, "%d", &ppm.max);
    ppm.size = ppm.width * ppm.height;
    ppm.pixels = (struct Pixel**)malloc(ppm.width * sizeof(struct Pixel));

    for (int i=0; i<ppm.width; i++) {
        ppm.pixels[i] = (struct Pixel*)malloc(ppm.height*sizeof(struct Pixel));
    }

    for (int i=0; i<ppm.width; i++) {
        for (int j=0; j<ppm.height; j++) {
            fscanf(f, "%d %d %d", &ppm.pixels[i][j].r, &ppm.pixels[i][j].g, &ppm.pixels[i][j].b);
        }
    } 
    return &ppm;
}

void showPPM(struct PPM * im) {
    printf("%s\n", ppm.ppmname);
    printf("%s", ppm.comments);
    printf("%d %d\n", ppm.width, ppm.height);
    printf("%d\n", ppm.max);

    for (int i=0; i<ppm.width; i++) {
        for (int j=0; j<ppm.height; j++){
            printf("%d %d %d\n", ppm.pixels[i][j].r, ppm.pixels[i][j].g, ppm.pixels[i][j].b);
        }
    }
}

struct PPM *encode(struct PPM *im, char *message, unsigned int mSize, unsigned int secret) {
    int *binaryMessage = decimalToBinary(message, mSize);
    int i, j, k;
    bool unique;
    srand(secret);
    int used[40]; memset(used, -1, sizeof(used)); // might have the last two args backwards... I always forget lol
    for(i = 0; i < 39; i+=3) {
        // determine next pixel based on the user's secret (srand)
        // and without ovewrwriting a previous hidden message
        do {
            unique = true;
            j = rand() % 3; // get new number and then check if it's unique
            for(k = 0; k < i; k++) if (used[i] == j) {unique = false; break;}
        } while(!unique);
        im->pixels[j]->r = im->pixels[j]->r & ~1 | binaryMessage[i+0]; // ~1 is 11111110
        im->pixels[j]->g = im->pixels[j]->g & ~1 | binaryMessage[i+1]; // anding keeps the first 7 and clears the last
        im->pixels[j]->b = im->pixels[j]->b & ~1 | binaryMessage[i+2]; // oring will set it if the binaryMessage is set
    }
        // should make a function...
        do {
            unique = true;
            j = rand() % 3; // get new number and then check if it's unique
            for(int k = 0; k < 39; k++) if (used[i] == j) {unique = false; break;}
        } while(!unique);
    im->pixels[j]->r = im->pixels[j]->r & ~1 | binaryMessage[39]; // last bit
    free(binaryMessage);
    return im;
}

static int *decimalToBinary(const char *message, unsigned int length) {
    /*
     * malloc is used here instead of [] notation to allocate memory,
     * because defining the variable with [] will make its scope
     * limited to this function only. Since we want to access this
     * array later on, we use malloc to assign space in the memory
     * for it so we can access it using a pointer later on.
     */
    int k = 0, i, j;
    unsigned int c;
    unsigned int *binary = malloc(8 * length * sizeof(int));

    for(i = 0; i < length; i++) {
        c = message[i];
        for(j = 7; j >= 0; j--,k++) {
            /*
             * We check here if the jth bit of the number is 1 or 0
             * using the bit operator &. If it is 1, it will return
             * 1 because 1 & 1 will be true. Otherwise 0.
             */
            if((c >> j) & 1)
                binary[k] = 1;
            else
                binary[k] = 0;
        }
    }
    return binary;
}

I was not sure why it occurred so I ran the error inside valgrind because I read online that it is a very useful debugger however I still cannot understand what the trace of the problem is. Here is the error report in valgrind:

==2066== Invalid free() / delete / delete[] / realloc()
==2066==    at 0x4C30D3B: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2066==    by 0x108B14: main (in /home/ziyad/test12)
==2066==  Address 0x30a060 is 0 bytes inside data symbol "ppm"
==2066== 
==2066== 
==2066== HEAP SUMMARY:
==2066==     in use at exit: 2,150,955 bytes in 515 blocks
==2066==   total heap usage: 519 allocs, 5 frees, 2,156,305 bytes allocated
==2066== 
==2066== 3 bytes in 1 blocks are still reachable in loss record 1 of 4
==2066==    at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2066==    by 0x108B4D: getPPM (in /home/ziyad/test12)
==2066==    by 0x108AC8: main (in /home/ziyad/test12)
==2066== 
==2066== 552 bytes in 1 blocks are still reachable in loss record 2 of 4
==2066==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2066==    by 0x4EBAE49: __fopen_internal (iofopen.c:65)
==2066==    by 0x4EBAE49: fopen@@GLIBC_2.2.5 (iofopen.c:89)
==2066==    by 0x108A9B: main (in /home/ziyad/test12)
==2066== 
==2066== 6,144 bytes in 1 blocks are still reachable in loss record 3 of 4
==2066==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2066==    by 0x108C0E: getPPM (in /home/ziyad/test12)
==2066==    by 0x108AC8: main (in /home/ziyad/test12)
==2066== 
==2066== 2,144,256 bytes in 512 blocks are still reachable in loss record 4 of 4
==2066==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2066==    by 0x108C53: getPPM (in /home/ziyad/test12)
==2066==    by 0x108AC8: main (in /home/ziyad/test12)
==2066== 
==2066== LEAK SUMMARY:
==2066==    definitely lost: 0 bytes in 0 blocks
==2066==    indirectly lost: 0 bytes in 0 blocks
==2066==      possibly lost: 0 bytes in 0 blocks
==2066==    still reachable: 2,150,955 bytes in 515 blocks
==2066==         suppressed: 0 bytes in 0 blocks
==2066== 
==2066== For counts of detected and suppressed errors, rerun with: -v
==2066== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

Answer 1

ppm is a stack object, not a heap-allocated one. You should not free it. You must only call free() on pointers previously returned by malloc() or calloc(). This is not the case here, so don't free() it.