Recovers jpg correctly, but .jpg not found [Recover]

Question

When running my code for recover (attached below) I'm bouncing back and forth between a segmentation fault and another error when running check50. My code runs without issue on card.raw

But when I try to run check50, I get ":( recovers 000.jpg correctly 000.jpg not found :( recovers middle images correctly 001.jpg not found :( recovers 015.jpg correctly 015.jpg not found"

I know the way I tried to break out of the large while loop was weird, but I didn't know how else to do it. We were warned repeatedly about not using while(!EOF) because it always goes for one too many loops. But fread-ing in and then using fseek to back up again is a slow way of going about solving the issue.

I know that the memory that is used to confirm our code in check50 is different than card.raw, but I don't understand why my code wouldn't work on another batch of memory formatted in the same way.

Any help would be greatly appreciated!

#include <cs50.h>
#include <stdio.h>
#include <stdint.h>

#define BLOCK 512

typedef uint8_t (BYTE);

int main(int argc, char** argv)
{
if (argc != 2)
{
    fprintf(stderr, "Usage: ./recover image\n");
}

FILE *file = fopen(argv[1], "r");
if (file == NULL)
{   return 1;   }


int counter = 0;

FILE *img = NULL;

BYTE buffer[BLOCK];


while(fread(&buffer,BLOCK,1,file)==1)
{

fseek(file,-BLOCK,SEEK_CUR);

fread(&buffer, BLOCK,1, file);

bool start = 0;

while( buffer[0] == 0xff &&
        buffer[1] == 0xd8 &&
        buffer[2] == 0xff &&
        (buffer[3] & 0xf0) == 0xe0)
        {
            start=1;
            break;
        }

while(start == 1)
{
    if (counter != 0)
        {fclose(img);}
    char filename[8];
    sprintf(filename, "%03i.jpg",counter);

    img = fopen(filename, "w");
    counter++;
    start = 0;
    break;
}

if(counter > 0)
{
    fwrite(&buffer, 1, BLOCK, img);
}

else
{
    fseek(file, BLOCK, SEEK_CUR);
}
}

fprintf(stdout,"You have reached the end of your files!\n");
        return 0;

}

Answer 1

This part of your code is unnecessary:

fseek(file,-BLOCK,SEEK_CUR);

fread(&buffer, BLOCK,1, file);

Having fseek in the while condition already reads 512 bytes and stores it into buffer, so the above code is equivalent to overwriting buffer with the same information.

The use of a while loop here is wrong:

while( buffer[0] == 0xff &&
        buffer[1] == 0xd8 &&
        buffer[2] == 0xff &&
        (buffer[3] & 0xf0) == 0xe0)
        {
            start=1;
            break;
        }

You should be using an if condition. I believe you meant this as a way to go through the file until the first JPEG is found. If that is indeed the case, then you should be doing something like this:

 while (start == 0)
        {
            // if the first jpeg is found
            if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && 
             (buffer[3] & 0xf0) == 0xe0)
                // set flag as non-zero to signal that first jpg is found
                start = 1;
        }

This will automatically break out of the while loop when start is changed to 1.

The rest of your code does not seem well thought out either. I would suggest you use GDB (and even pen and paper) to figure out how each step of your algorithm gets executed.