Pset4 - Recover - Segmentation fault when trying to free buffer

Question

I have run the debug50 and it seems the seg fault occurs after trying to free memory here:

 // if not start of a new jpeg and we HAVE NOT already found one before

  if (block_buffer[0] != 0xff && jpegcounter == 0)
    {
    //free buffer
    free(block_buffer);
    }

I did first try setting my buffer on the heap using:

unsigned char* block_buffer = malloc(sizeof(unsigned char) * 512);

However I got memory leaks so am proceeding with array for now.

I have copied my code below any hints or ideas on what might be causing problems would be much appreciated. I expect there a few things wrong as it usually takes me a few drafts, but really stuck with this seg fault.

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

int main(int argc, char *argv[])
{
    //declare variables
    int jpegcounter = 0;
    int zerocounter = 0;//used to find the EOF (if a block contains all zeros then this should = 512)
    int i;
    char filename[8]; //stores 000.jpeg format
    FILE* img;


 // ensure proper usage (can only accept one commant line argument)
    if (argc != 2)
    {
        fprintf(stderr, "Enter 1 argument only\n");
        return 1;
    }

    //give filename to input
    char* card_raw = argv[1];

//open card file
FILE *cardfile = fopen(card_raw, "r");

//ensure that file can be openned for reading
if (cardfile == NULL)
    {
        fprintf(stderr, "Could not open %s.\n", card_raw);
        return 2;
    }

//repeat until end of card (logic being that a block containing all zeros will neither be new jpeg or part of an  already open one)
while(zerocounter != 512)
{

//allocate array for block buffer
unsigned char block_buffer[512];

//read 512 bytes into a buffer
fread(block_buffer, sizeof(unsigned char), 512, cardfile);

//start of a new JPEG? - YES and not found one before
    if (block_buffer[0] == 0xff &&
        block_buffer[1] == 0xd8 &&
        block_buffer[2] == 0xff &&
        (block_buffer[3] & 0xf0) == 0xe0

    && jpegcounter == 0)
    {
        //send formatted filename to array (filname[8])

        sprintf(filename, "%03i.jpeg", jpegcounter);
        img = fopen(filename, "w");

        //write bytes to img file until a new jpeg is detected
        fwrite(block_buffer, sizeof(unsigned char), 512, img);

        //free buffer
        free(block_buffer);
    }

// if not start of a new jpeg and we HAVE already found one before

    if (block_buffer[0] != 0xff && jpegcounter > 0)
    {
        sprintf(filename, "%03i.jpeg", jpegcounter);
        img = fopen(filename, "w");

    //write bytes to img as part of same filename
        fwrite(block_buffer, sizeof(unsigned char), 512, img);

    //free buffer
    free(block_buffer);
    }

//if new jpeg and have found one before

if (block_buffer[0] == 0xff &&
    block_buffer[1] == 0xd8 &&
    block_buffer[2] == 0xff &&
    (block_buffer[3] & 0xf0) == 0xe0

    && jpegcounter > 0)

  {
      jpegcounter ++;

//Close last img file
    fclose(img);
    sprintf(filename, "%03i.jpeg", jpegcounter);
    img = fopen(filename, "w");

    //write bytes to img as part of same filename
        fwrite(block_buffer, sizeof(unsigned char), 512, img);

    //free buffer
    free(block_buffer);
  }

  // if not start of a new jpeg and we HAVE NOT already found one before

  if (block_buffer[0] != 0xff && jpegcounter == 0)
    {
    //free buffer
    free(block_buffer);
    }


// if not start of a jpeg and contains zeros
else
    {
        for (i = 0; i < 512; i++)

        //initialise zerocounter to 0 each time condition checked
        zerocounter = 0;

        {
            if (block_buffer[i] == 0x00)
            {
                zerocounter ++;
            }
        }

    if (zerocounter != 512)
    //write bytes to img as part of same filename
        {
            fwrite(block_buffer, sizeof(unsigned char), 512, img);

            //free buffer
            free(block_buffer);
        }

}

}//end while loop

}//end main

Answer 1

block_buffer in your code is an array on stack, not dynamically allocated on the heap. You would only free what you malloced before (or equivalent).

The nice part of stack variables is that they cease to exist when reaching the end of the block they are declared in, so no need to free, all automatic. The stack pointer (which is used to remember where the next stack variable would go) is reset to the value of before the code block, and it's as if the variable had never existed. Just that its value is still there, and another variable at that memory location would have it for initial value, which is why you should always take care to initialise your variables.

In some cases, we cannot use an array on stack, e.g. if we need to resize the array (there's no equivalent to realloc), or if the buffer needs to exist even after this block, e.g. is returned by the function. In that case, one would either have a stack variable in a calling function and pass that to be used, or combine a malloc (or equivalent) with a free, ideally in the same function (or be clear the caller is responsible for freeing the allocated memory). It's important to free any allocated memory exactly once. In our case, that could be achieved with by having a single unsigned char *block_buffer = malloc(512 * sizeof(unsigned char)); before the loop, and a free(block_buffer); just before you return. Just that it's extra work here, and an array on stack works great. Please don't try to allocate a new buffer for each read block, the memory you got does not wear out from re-using it.

BTW, I don't think you should have more than one check of whether or not you just read a JPEG header. If it's a JPEG header, have another if inside that fcloses an open output file if it exists, the rest should be same code.