PSet 4 Image Recovery - Off-by-one error?

Question

The recover program seems to be working almost perfectly, but I think I must be missing the first file, somehow. I get images 000.jpg to 049.jpg, and all of them are openable, but with a few weird things happening:

-The second to last picture, 048.jpg (Hulk homage, I think), has tons of white space after it. This makes me think this should actually be the last picture. I have an idea for how I can get it to not add so much white space, but that's not the big problem.

-049.jpg opens, but it is not really a picture:

-So, this makes me think that the picture I have as 000.jpg (A woman in a black parka in the snow) is actually the second picture in the file, not the first one.

Here's the code:

/**
 * recover.c
 *
 * Computer Science 50
 * Problem Set 4
 *
 * Recovers JPEGs from a forensic image.
 */
#include<cs50.h>
#include<stdio.h>
#include<stdlib.h>
#include<stdint.h>


int main(int argc, char* argv[])
{
FILE* input = fopen("card.raw", "r");
FILE* output;

//Ok, gonna need some variables that persist:
int filecount = 0;
uint8_t block1Check = 0xff;
uint8_t block2Check = 0xd8;
uint8_t block3Check = 0xff;
uint8_t block4CheckLow = 0xe0;
uint8_t block4CheckHigh = 0xef;
uint8_t wholeBlock[512];
bool recovering = false;


while (filecount < 49) //keep looking through the file until I've found all 50 jpegs
{

    //fetch the beginning of the block
    fread(&wholeBlock, sizeof(wholeBlock), 1, input);

    uint8_t block1 = wholeBlock[0];
    uint8_t block2 = wholeBlock[1];
    uint8_t block3 = wholeBlock[2];
    uint8_t block4 = wholeBlock[3];


    //If it's a jpeg signature, get the jpeg, and up the filecount by one
    if (block1==block1Check && block2==block2Check && block3==block3Check && ((block4>=block4CheckLow)&&(block4<=block4CheckHigh)))
    {
        if(!recovering)
        {
            char filename[3];
            sprintf(filename, "%03i.jpg", filecount);
            output = fopen(filename, "w");
            fwrite(wholeBlock, sizeof(wholeBlock), 1, output);
            recovering = true;
        }
        else
        {
            fclose(output);
            filecount++;
            char filename[3];
            sprintf(filename, "%03i.jpg", filecount);
            output = fopen(filename, "w");
            fwrite(wholeBlock, sizeof(wholeBlock), 1, output);
            recovering = true;
        }
    }
    else if(recovering)
    {
        fwrite(wholeBlock, sizeof(wholeBlock), 1, output);
    }


}

//make sure everything is closed
if(output)
{
    fclose(output);
}
fclose(input);
return 0;
}

Any help would be greatly appreciated.

Thanks,

-B

Answer 1

This is not an off by 1 problem. All of your files except the last are correct. Your problem lies in how the last file is handled.

Think about what happens when the last file is being processed. Your while condition will terminate when ( filecount < 49 ) is no longer true. Now, as the program processes the card.raw file, it will read the signature block for the last file, open that file, and write the first block of data into the file. It also increments filecount to 49 and circles back to the top of the while loop to start the process of reading the rest of the blocks of data for the image file. Unfortunately, filecount is now 49, so the while loop terminates before reading the rest of the file and writing the data to 049.jpg. The result is that you have a truncated image file with only 512 bytes.

The program needs to read the input file until it is completely processed. It's stopping after reading the signature block of the last image file. When you recode for this, be careful to avoid the common mistake of reading from the input file, processing the data and then checking for EOF.

As a side note, what if the file doesn't contain 50 images? What will your program do? If you were writing this program for work or to solve a universal problem, would you write it to recover a fixed number of files, or to recover all of the files it could? It might surprise you to know that check50 uses a different card.raw file that has a different number of files! Instead of hardcoding a fixed number of files, you really should code to recover an unknown number of files by reading to the end of the input file. ;-)

You could also improve efficiency in a couple ways. First, instead of having two blocks of code to open a file, you could do this: Check to see if an output file is already open. If so, close it. Now that there are no open output files, open one.

Second, don't write the data to the output file in the block that opens the file. Instead (and this is an if statement, not else if), if there's an open output file, write the data block.

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