Pset 4 : Recover, Segmentation fault

Question

I am having problems identifying where i have gone wrong in the below code. On running the program i am getting segmentation fault. i have gone through many related tags and answers but cant understand my mistake. Another thing i want to ask is the difference between the usage of &buffer and buffer in fread function as i am finding it confusing. Hope you will be able to help me. Thank you.

#include <stdio.h>

int main(int argc, char *argv[])
{
if (argc != 2)
{
    fprintf(stderr, "program should be executed only with one command line arguement - the name of a forensic image from which to recover JPEGs\n");
    return 1;
}

FILE *in = fopen(argv[1], "r");

if (in == NULL)
{
    fprintf(stderr, "the forensic image cannot be opened\n");
    return 2;
}

typedef unsigned char BYTE;
BYTE buffer[512];
int images = 0;
char *filename = NULL;
FILE *img = NULL;

while (fread (&buffer, 1, 512, in) == 512)
{

    if (buffer[0] == 0xff &&
        buffer[1] == 0xd8 &&
        buffer[2] == 0xff &&
        (buffer[3] & 0xf0) == 0xe0)
    {
        if (images != 0)
        {
            fclose(img);
        }
        sprintf(filename, "%03i.jpg", images);
        img = fopen (filename, "w");
        if (img == NULL)
        {
            printf("There was a problem, could not create the file\n");
            return 1;
        }
        images++;
        fwrite(&buffer, 1, 512, img);
    }
    else if (images > 0)
        {
            fwrite(&buffer, 1, 512, img);
        }
    fseek(in, 512, SEEK_CUR);
}
fclose(img);
fclose(in);
}

Answer 1

Your segfault happens on sprintf. You need to provide at least 8 bytes to store the file name. 7 for the characters, 1 for the null terminator. I'd use an array like you do for the buffer.

There's a second potential segmentation fault, when you fclose the output file at the end. Will crash if you never opened one, put some if (images > 0) in front to avoid this.

buffer and &buffer are the same in case of a stack array, pointers to the first element. If you want proof, just try

printf("buffer: %p\n&buffer: %p\n", buffer, &buffer);

If you had declared it like BYTE *buffer = malloc(512); (allocating on heap instead of stack), then those would differ, with buffer pointing to the 512 bytes, and &buffer pointing to the 8 byte pointer pointing to the 512 byte range. In that case, it would be important to use buffer.

BTW, the fseek makes no sense. Any read operation already advances the position within the file.