PSET 4 recover doesn't pass check50 - “recovered images do not match”

Question

All images are recovered, and when I open them, they all display correctly (I checked every one of them in the IDE), yet my program is not passing check50. Why? Here's my code:

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

int main(int argc, char *argv[])
{

    // ensure proper usage
    if (argc != 2)
    {
        fprintf(stderr, "Usage: ./recover image\n");
        return 1;
    }

    // create pointer to specified file
    FILE *file = fopen(argv[1], "r");
    if (file == NULL)
    {
        fprintf(stderr, "Could not open %s.\n", argv[1]);
        return 2;
    }

    unsigned char buffer[512];
    int numOfPics = 0;
    while(1)
    {
        fread(&buffer, 1, 512, file);

        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
        {
            char buffer2[10];

            // create a new JPEG and increment the number of pictures read
            sprintf(buffer2, "%03i.jpg", numOfPics);
            numOfPics++;
            FILE *img = fopen(buffer2, "w");

            // write the header to new JPEG, then write the remaining bytes
            fwrite(buffer, 1, 512, img);

            do
            {
                fread(buffer, 1, 512, file);
                fwrite(buffer, 1, 512, img);
                if (feof(file))
                {
                    fclose(file);
                    return 0;
                }
            } while (!(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0));

            fclose(img);
            fseek(file, -512, SEEK_CUR);
        }
    }
    return 0;
    fclose(file);
}

EDIT: here's my updated code

unsigned char buffer[512];
    int numOfPics = 0;

        while(fread(&buffer, 1, 512, file))
        {
            char buffer2[10];
            if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
            {
                // create a new JPEG and increment the number of pictures read
                sprintf(buffer2, "%03i.jpg", numOfPics);
                numOfPics++;
                FILE *img = fopen(buffer2, "w");

                // write the header to new JPEG, then write the remaining bytes
                fwrite(&buffer, 1, 512, img);

                fclose(img);
            }
            else
            {
                if (feof(file))
                {
                    fclose(file);
                    return 0;
                }
                FILE *img = fopen(buffer2, "a");
                fwrite(&buffer, 1, 512, img);
                fclose(img);
            }
        }

EDIT2:

unsigned char buffer[512];
int numOfPics = 0;
FILE *img = NULL;
char buffer2[10];

while (fread(&buffer, 1, 512, file))
{
    if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
    {
        // create a new JPEG and increment the number of pictures read
        sprintf(buffer2, "%03i.jpg", numOfPics);
        numOfPics++;
        if (img)
        {
            fclose(img);
        }
        img = fopen(buffer2, "a");

        // write the header to new JPEG
        fwrite(&buffer, 1, 512, img);

    }

    // write remaining bytes to JPEG
    else if (numOfPics > 0)
    {
        fwrite(&buffer, 1, 512, img);
    }
}
fclose(img);
fclose(file);
return 0;

Answer 1

This is a good example of issues hiding in plain sight. Yes, the images look right. But extraneous data at the end of an image file aren't displayed. Image file handlers will only display the amount of data that the header says are supposed to be in the file. There could be gigabytes appended to the file and you'd never see it! The lesson is to look everywhere for additional data. Look at the file size, look at the physical data in the file, in addition to the displayed image!

The code is reading data from the input file and writing to the output file. Here's the problem - when it hits a data block that has a signature, it writes that signature to the end of the current file and then opens a new file, where it writes the signature again. All of the files are 512 bytes too large.

Side note: while(1) -- This is a forever loop, usually a bad idea. Instead, why not incorporate something into the while condition that will end the loop under certain conditions. Seems like the fread would be a good choice, wouldn't it?

Here's another challenge for you. While the code above can be easily fixed to work, the code can be written with a single fread, a single while loop (no do loops), and no need for fseek. ;-)

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)