pset4 recover.c seg fault

Question

In my pseudo code this is what this will do(I made FILE* img a prototype also counter = 0 and newcounter is 2 initially ) read the first 512 bytes check if the header is in it if it is say found one. Increase counter and decrease newcounter. then check if counter is greater than new counter 1 is not > 1 and hence it will not close the file the first time a new jpeg is found. Then it will make a title as 001. Then it will check if counter is greater than 0 if it is open image name title and then write to img. It will loop again with next 512 bytes and then since counter is greater than 0 it will write 512 bytes more. However once a new header is found it will close the file and then restart the whole process. However I am getting a segfault after just making one jpeg with 0 bytes

this is my code

while(fread(buffer, 512, 1, card) == 1)
{
    fread(buffer, 512, 1, card);

    for(int c = 0; c < 512 ; c++)
    {
        if(c == 3 && (buffer[c] == 0xe1 || buffer[c] == 0xe0) && buffer[c - 1] == 0xff && buffer[c - 2] == 0xd8 && buffer[c - 3] == 0xff)
        {
            //new file detector
            printf("found one\n");
            counter++;
            newcounter--;
            if(counter > newcounter)
            {
                fclose(img);
            }
            sprintf(title, "%.3d.jpg", counter);
        }
    }

    if (counter != 0)
    {
        FILE* img = fopen(title ,"a");
        fwrite(buffer, 512, 1, img);
    }  
}

Answer 1

Let's start from the basic. On every loop, you are reading 2 chunks of 512 bytes into buffer, because there is one fread in the while loop, and one more right after. The fread in the condition stores the bytes in buffer too, you don't have to call another fread.

Then there is the thing about the start of a jpg again. I told you on a previous question of yours, that you only have to check the first 4 bytes in your buffer, but you are still using a for loop and conditions, checking all the possible 4 adjacent bytes in buffer.

I don't know what newcounter is, why you decrement it and why you need it at all.

I don't see the declaration of title, but if I had to guess, it doesn't have the necessary space to store all the characters given by sprintf. That has as a result that when you try to open the FILE* img, the name of the file is not in a right format, because it's not a complete string, so fopen returns NULL and since you don't check for NULL after the opening, (which should probably be invoked with "w" and not "a" so that you overwrite any previously produces images), when you try to write the 512 bytes, you try to write them at NULL and that's when you get the segfault.

Overall your code logic has many voids. Maybe you should rewatch the lecture videos and the walkthroughs to get it better.