PSet4 Recover, only finds 22 corrupted photos, doesn't pass Check50

Question

Originally, my code always returned Segmentation Faults whenever running it. I used printf statements to find and fix the segmentation faults, but now it is only finding 22 out of the 50 photos and are all corrupted/incomplete. I've been working on this problem for a couple of days and can't seem to identify where I'm making my mistakes! Any help would be appreciated!

Check50 Results: :) 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 049.jpg not found

include

include

include

int main(int argc, char *argv[]) { //Declaration typedef uint8_t BYTE; BYTE buffer[512]; char filename[8]; FILE *memorycard; FILE *img = NULL; int counter = 0;

//Check for correct command line usage
if (argc != 2)
{
    printf("Usage: ./recover image\n");
    return 1;
}

//Open memory card (inptr)
char *file = argv[1];
memorycard = fopen(file, "r");

//Check if memorycard OR argv[1] is empty

if (memorycard == NULL)
{
    printf("Could not open %s\n", argv[1]);
    return 1;
}

//Loop until end of 512 block
while (fread(buffer, sizeof(BYTE), 512, memorycard) == 512)
{
    //Read 512 bytes into buffer
    fread(buffer, sizeof(BYTE), 512, memorycard);

    //If JPEG header is found
    if (buffer[0] == 0xff &&
        buffer[1] == 0xd8 &&
        buffer[2] == 0xff &&
        (buffer[3] & 0xf0) == 0xe0)
    {
        //If first JPEG (counter == 0)
        if (counter == 0)
        {
            sprintf(filename, "%03i.jpg", counter); //Create new filename to write into
            img = fopen(filename, "w"); //Open img (outptr) to put filename into
            fwrite(buffer, sizeof(BYTE), 512, img); //Write
            counter++;
        }
        //If not first JPEG
        else if (counter > 0)
        {
            //Close open filename
            fclose(img);

            sprintf(filename, "%03i.jpg", counter); //Create new filename to write into
            img = fopen(filename, "w"); //Open img (outptr) to put filename into
            fwrite(buffer, sizeof(BYTE), 512, img); //Write
            counter++;

        }
    }

    //If no JPEG header is found
    else
    {
        //Continue writing in to img (outptr)
        if (img != NULL)
        {
            fwrite(buffer, sizeof(BYTE), 512, img); //Write
        }
    }
}

//Close all remaining files
fclose(img);
fclose(memorycard);

}

Answer 1

This is a pretty common newbie mistake. Look at this code:

//Loop until end of 512 block
while (fread(buffer, sizeof(BYTE), 512, memorycard) == 512)
{
    //Read 512 bytes into buffer

fread(buffer, sizeof(BYTE), 512, memorycard);

This will read 512 bytes into the buffer, and then read another 512 bytes, overwriting the data from the first read. Simply put, it's reading and throwing away every other 512 bytes that are read.

The common error is that people think that the read inside the while statement only tests for EOF but doesn't actually execute a read. In reality, it actually executes a read and returns a value to be tested. Every fread returns a value that may or may not be used, depending on how the code is written.

So, if you start by deleting the second fread statement, you should be well on your way. I didn't check for any other errors, so there may be other problems, but they'd be for a new question. ;-)

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