CS50 Recover Problem- recovered images do not match

Question

Results for cs50/problems/2020/x/recover generated by check50 v3.1.2 :) recover.c exists.
:) recover.c compiles.
:) handles lack of forensic image
:( recovers 000.jpg correctly recovered image does not match
:( recovers middle images correctly recovered image does not match
:( recovers 049.jpg correctly recovered image does not match

I don't know why I'm facing this issue. Please help me with this it might be because I'm not clear with the file fundamentals or something.

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

typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    //to check for command-line arguments
    if( argc !=2 )
    {
        printf("Usage: ./recover imagename");
        return 1;
    }
    
    //file pointer from where to read
    FILE *inptr = fopen(argv[1], "r");

    if( inptr == NULL)
    {
        fprintf(stderr, "File couldn't open");
        return 1;
    }

    BYTE buffer[512]; //buffer for storing input data from file
    char FILENAME[8]; //output file name storage
    int counter=0; //to handle naming of file
    FILE *outptr = NULL; //file pointer where to write

    while(fread(buffer, 512, 1, inptr))
    {
        if(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
        {
            sprintf(FILENAME, "%03i.jpg", counter);
            outptr = fopen(FILENAME, "a");
            if(outptr != NULL)
            {
                fclose(outptr);
                counter++;
            }
        }
        if(outptr != NULL)
        {
            fwrite(buffer, 512, 1, outptr);
        }
    }
    fclose(outptr);
    fclose(inptr);
}

Answer 1

Your code has one block out of sequence that results in you creating empty files. In your code you have the following:

        sprintf(FILENAME, "%03i.jpg", counter);
        outptr = fopen(FILENAME, "a");
        if(outptr != NULL)
        {
            fclose(outptr);
            counter++;
        }

Where you create the new filename with sprintf() and then open the file with fopen(). If the open succeeds, you immediately close the file with fclose() and increment counter before anything is written to the file. Not what you want.

Instead, you want to check if you have an output file currently open, if so, you want to close the current file, then open the next output file for writing. (recommend opening with mode "wb" instead of "a"). Rearranging, you would have:

            if(outptr != NULL)  /* if output file open, close before opening next */
            {
                fclose(outptr);
                counter++;
            }
            sprintf(FILENAME, "%03d.jpg", counter);
            outptr = fopen(FILENAME, "wb");     /* open in write, not append mode */

Now your file will remain open so you can write the recovered jpg to the file.

Avoid Hardcoding Filenames or Using Magic-Numbers

You do fine with your filenames, but you should declare constants for numbers used in your code. Why? When you #define a constant at the top of your code, you provide a single convenient location to make adjustments to any of the fixed values without having to pick through all loop limits and function calls just to make a change. Further, when declaring buffers for filenames, etc.., Don't skimp on buffer size.... It's far better to have a buffer that is 1000 characters too long that have it be one character too short.

You can simply use #define to define one or more constants or use a global enum for the same purpose. Here you could do:

#define BLKSZ  512      /* if you need a constant, #define one (or more) */
#define MAXFN 1024      /*         (don't skimp on buffer size)          */
...

And then your use would be:

    ...
    BYTE buffer[BLKSZ];     /* don't use MagicNumbers, use a constant */
    char FILENAME[MAXFN];   /* ditto */
    ...
    while (fread(buffer, BLKSZ, 1, inptr))
    {
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && 
            buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
        {
            if(outptr != NULL)  /* if output file open, close before opening next */
            {
                fclose(outptr);
                counter++;
            }
            sprintf(FILENAME, "%03d.jpg", counter);
            outptr = fopen(FILENAME, "wb");     /* open in write, not append mode */
        }
        if(outptr != NULL)
        {
            fwrite(buffer, BLKSZ, 1, outptr);
        }
    }

(note: you want to control long lines by breaking them with a newline to prevent unnecessary line wrapping that can make the code hard to read. Recently the Linux kernel style guide raised the recommended characters per-line from 80 to 100 characters -- though, here on StackOverflow you will note your lines began to require a scroll bar a few characters past 90)

Now if anything changes, you have two simple constants to change.

With the reordering of the fclose() call, your program now recovers all 50 jpg files from card.raw just fine.

Let me know if you have further questions.