0

It looks like this will be one of many questions regarding this already on CS50SE... however I just can't figure this out at all despite looking through all the other questions of people who have this problem and trying the suggestions with my code.

All the images are recovered OK but check50 can't see them:

:) recover.c exists.
:) recover.c compiles.
:) handles lack of forensic image
:( recovers 000.jpg correctly
    000.jpg not found
:( recovers middle images correctly
    001.jpg not found
:( recovers 015.jpg correctly
    015.jpg not found

Here is my program:

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

int main(int argc, char *argv[])
{
// ensure correct usage
if (argc != 2) {
    fprintf(stderr, "Usage: ./recover infile (card.raw)\n");
    return 1;
}

char *infile = argv[1];

// open input file
FILE *inptr = fopen(infile, "r");
if (inptr == NULL) {
    fprintf(stderr, "Could not open %s.\n", infile);
    return 2;
}

uint8_t buffer[4]; // setting up array for JPEG header hexes (ff d8 ff)
uint8_t currentByte; // setting up uint8_t size variable for reading the hex from the infile, storing it here and then writing it to the outfile.

do {
    fread(&currentByte, 1, 1, inptr);
}
while (currentByte != 0xff);

if (currentByte == 0xff) { // if current byte in infile = 0xff
    buffer[0] = 0xff;
    for (int i = 1; i <= 3; i++) {     // counting from 1 as position zero is spoken for in line above :)
        fread(&currentByte, 1, 1, inptr);
        buffer[i] = currentByte;               // buffer index i = current Byte being read.
    }
}

if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0) {
    fseek(inptr, -4, SEEK_CUR); // seek back 4x hexes to the start of the JPEG header

    for (int i = 0; i <= 3; i++) {   // empty the buffer array that checks for JPEG headers
        buffer[i] = 0;
    }

int fileNameCounter = 0;
FILE* outptr = NULL;

while(!feof(inptr)) {

    if(fileNameCounter > 100) {
        return 1;
    }

    char outfile[8];

    if (fileNameCounter < 10) {
        sprintf(outfile, "00%i.jpg", fileNameCounter);
    }
    else {
        sprintf(outfile, "0%i.jpg", fileNameCounter);
    }

    outptr = fopen(outfile, "w");

    int counter = 0;
    int writeOK = 0;

    for (int i = 0; i <= 3; i++) { // empty the buffer array that checks for JPEG headers
        buffer[i] = 0;
    }

    do {
            writeOK = 1;

            if(!feof(inptr)) {
                fread(&currentByte, 1, 1, inptr);
            }
            else {
                return 0;
            }

            if (currentByte == 0xff && counter > 0) { // if current byte in infile = 0xff
                buffer[0] = 0xff;
                for (int i = 1; i <= 3; i++) { // counting from 1 as position zero is spoken for in line above :)
                if(!feof(inptr)) {
                    fread(&currentByte, 1, 1, inptr);
                }
                else {
                    return 0;
                }
                    buffer[i] = currentByte; // buffer index i = current Byte being read.
                    }
                if (!(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)) {
                    fseek(inptr, -4, SEEK_CUR);

                    if(!feof(inptr)) {
                        fread(&currentByte, 1, 1, inptr);
                    }
                    else {
                        return 0;
                    }
                    writeOK = 1;
                }
                else {
                    fseek(inptr, -4, SEEK_CUR);
                    writeOK = 2;
                    counter = 0;
                }
            }

            if (writeOK == 1) {
                fwrite(&currentByte, 1, 1, outptr);
                counter++;
            }
    }
    while(!(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0));

    fclose(outptr);
    fileNameCounter++;
}
fclose(inptr);
}

// end of script, return 0
return 0;
} // closing main

Any suggestions you can provide would be fantastic, it's driving me nuts! Thanks in advance! :)

1 Answer 1

1

In this case, I'm going to do something different and provide more of a critique than a pointer at the problem(s). There are a number of issues, but first, something to think about. Also, this is not at all to be offensive or to ridicule. It's to correct issues before they become habits and to help you become a better programmer. You can't fix it if you don't know what it is!

The check50 program actually uses a different data set to test the submitted solution. The hidden lesson here is that it is necessary to do exhaustive testing on code. How do you know whether the test data is sufficient? Interestingly, this is the first time I've ever seen a program work on the provided data and completely fail to recover a single file from the alternate data set.

To get to the core of the issue, the code is simply too complex and should be simplified. Also, it is clear that part of the explanation of what was needed was not fully grasped. Let's cover that now. The signature that marks the start of a file starts at the beginning of a 512 byte block of data, not in the middle, or anywhere else - only on a 512 byte boundary. Searching one byte at a time for the beginning ff of a signature is totally unnecessary and inefficient. Worse, it could potentially find a false signature. It is entirely possible that the internal data of an image file or the garbage leading data could contain an intermediate string of data that matches a valid signature, leading to a false positive. In fact, there is one set of test data that does exactly that! So, the only data that should and must be checked are the first 4 bytes of each 512 bytes.

Next, there are inefficiencies to deal with. In different places, the code reads one byte at a time from the file, fseeks backwards, rereads the same data, etc. In this pset, it is only necessary to read the data once and to do so in a linear fashion, 512 bytes at a time. Here's a little fun fact that you may not know. A read has a lot of overhead, especially with hard drives that have spinning disks. Most of the execution time of a read is spent in overhead, positioning the head on the right track and spinning around to the right position on the track. It then reads a block of data. The overhead can be as much as 95% or more of the read time. Now, compare reading 1 byte at a time to 512 bytes at once. It can be 500 times faster or more! ( SSD drives and flash memory with no moving parts and electronic storage are much faster and don't suffer from this quite as bad.)

In line with this, copying the data from one buffer currentbyte to another buffer[] is wasteful.

Why not just read in a block of 512 bytes at once directly into a 512 byte buffer, check the first four bytes, and either ignore the data, or open a file if necessary and write out the entire block.

Next, there are a number of nested do and do/while loops. An efficient version of this program can be written with a single while loop, one 512 byte buffer and no fseek calls.

As to why this program doesn't work with the check50 data, I'll be honest. I don't know. Because of the complexity of the code, I didn't want to dig very far into it to find out. It's late and I already have a headache to the point that I can't focus. It's a poor excuse, but I wanted to give you some advice on coding. There's an old saying in programming that, sooner or later, I have to remind myself on every project: "KISS - keep it simple, stupid!" It usually follows a period of writing code that starts to get too convoluted while trying to fix a bug that's buried deep. ;-) If you keep things simple, the risk of errors or bugs creeping into your code is minimized.

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

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .