Problem Set 4 Recover

Question

I have written a solution to CS50's Problem Set 4 Recover, and my code produces all 50 images clearly (to me, at least). However, check50 says that the middle images and last images do not match what is correct. I have spent many hours trying to figure out what the problem is by manually adding printf statements with ftell, but I have not been able to identify the issue. It is especially puzzling why check50 says the first image, 000.jpg, is correct, as when I have tried to card-code the 10240 byte picture this latter method fails. I am very grateful for any advice or suggestions anyone may have. Thank you.

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

#include <stdint.h>

typedef uint8_t BYTE;

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

    BYTE buffer[512];
    BYTE search[4];

    if (argc != 2)
    {
        printf("Usage: ./recover [forensic image to be examined]\n");
        return 1;
    }

    FILE *input = fopen(argv[1], "r");
    if (input == NULL)
    {
        printf("Forensic image cannot be opened for reading.\n");
        return 1;
    }

    // Search for beginning of the jpg
    fread(buffer, 1, 512, input);
    int grand_counter = 0;
    int searcher = 0;
    int orig_holder;
    int upd_holder;

    while (0 == 0)
    {
        // Search apparatus
        if (buffer[searcher] == 255)
        {
            orig_holder = ftell(input);
            fseek(input, orig_holder - 512 + searcher, SEEK_SET);
            upd_holder = ftell(input);
            fread(search, 1, 4, input);
            if (search[1] == 216 && search[2] == 255 && search[3] >= 224 && search[3] <= 239)
            {
                fseek(input, upd_holder, SEEK_SET);
                break;
            }
            fseek(input, orig_holder, SEEK_SET);
        }
        searcher++;
        if (searcher == 512)
        {
            searcher = 0;
            fread(buffer, 1, 512, input);
        }
    }

    // Begin reading files
    fread(buffer, 1, 512, input);

    char *name = malloc(8);
    int count = 0;
    int indicator = 1;
    FILE* output;

    int index = 0;


    name[0] = 48;
    name[1] = 48;
    name[2] = 48;
    name[3] = '.';
    name[4] = 'j';
    name[5] = 'p';
    name[6] = 'g';
    name[7] = '\0';

    for (int i = 48; i < 53; i++)
    {
        name[1] = i;
        for (int j = 48; j < 58; j++)
        {
            name[2] = j;
            output = fopen(name, "w");

            while (indicator == 1)
            {
                // Perform read-write function
                fwrite(buffer, 1, 512, output);
                fread(buffer, 1, 512, input);

                // Check to see if additional material exists
                if (buffer[0] == 255 && buffer[1] == 216 && buffer[2] == 255 && buffer[3] >= 224 && buffer[3] <= 239)
                {
                    indicator = 0;
                }

                if (ftell(input) == 506368)
                {
                    fwrite(buffer, 1, 512, output);
                    indicator = 0;
                }
            }

            fclose(output);
            indicator = 1;
        }
    }

    // Liberate all memory
    free(name);
    fclose(input);
}

Answer 1

The issue is that your code is written to specifically handle only the one raw file that is given to you as an example. It needs to handle any raw file. (check50 is testing your code with a different one)

Answer 2

At a quick glance, I'd say that this is your problem, or at least a part of it. This code has the size of the input file hard-coded to decide when to end the program. WHY is it a problem? Check50 uses a different input file that's a different size! It's designed to catch exactly this issue.

If you were writing a real-world utility program to do this, there's no way to know the exact size of any input file. The program should contain code that would handle detecting the end of the input file, ie, an EOF condition. Hard-coding the input file size is, well, cheating in my opinion. You could write code to determine the size of any input file size, but that's just a variation on the theme. Why bother checking the file size and then adding extra code to detect when that file size has been processed? Instead, the code could just take the input one block at a time and process it until it hits the end of the file. It's a linear read of the input file, so why waste time calculating the file size when the code simply must process the data from start to finish, without skipping around. Also, it's a lot more efficient to check the EOF flag status than to check pointer positions and compare to the determined file size.

I have to comment on the code itself. It's far too complicated and convoluted for what it needs to do. Anyone reading the code will have to pay very close attention to understand what's going on, particularly given the absence of any comments! Imagine coming back in 6 months or a year to fix a bug that's just been discovered. What was the logic of all this code???? This complexity has also caused the program to be at least 50% larger than necessary.

While most of the code (perhaps almost all) looks to be technically right, it's unnecessarily complicated. Some examples:

The output filename string var: name. There are 8 lines of code just to initialize this filename var! It could be done in 1 line with a strcpy(). Truth is, it doesn't need to be initialized there (lines 69-76). It can be initialized just before it's first use.

Related to that, creating two nested FOR loops to handle creating the filenames is overly complex coding. Instead, just call a strcpy() command when the filename needs to be set.

The initial code block that searches for the first signature data block (lines 27 to 57 or so) could be eliminated completely, with some logic that's more efficient.

Wouldn't it be a lot easier to use a sprintf() call with a formatting string in the statement?

        // Create a new output filename, 
        // where jpeg is the int var containing the next file number

        sprintf( outfile, "%03d.jpg", jpeg++);

From the assignment, it's given that the beginning of the input file may be garbage data and should be discarded until the first signature block is found. (BTW, what if there isn't any garbage data?) After the garbage data, the program should read in one 512 byte block at a time and output it to a jpg file until the next signature block is found.

The simple logic here is this: Read in a block of data. If it doesn't have a signature, it should either be discarded or written out to the currently open output file. Think about this point. To write out to a file, an output file has to be open. For an output file to have been opened, a signature block has to have been found! If no signature block has been found yet, there's no open file, so don't even try to write the data out. Just loop around and process the next 512 byte block where the buffer will simply overwrite the garbage data block. There's no need to write a separate block of code just to skip over the garbage data.

Here's another tip - more of a challenge to you. The code above used 5 fread statements and two fwrite statements. It's possible to write a more efficient version of this program using exactly 1 fread and 1 fwrite call each. Think you can do it? ;-)

A couple more tips. Learn to use hex values. Most programmers get very used to that and prefer it, especially when they have to look at raw data and compare it to code. Imagine this: You're trying to debug this program and need to look at the raw values of a 512 byte block of data. You're looking at the data in debug mode or in a hex editor to see the raw values in the file. The data is going to appear in hex, not decimal. Would it be easier to compare the first 4 bytes of the buffer (displayed in hex) to 0xFF,0xD8,0xFF,0xE0 thru 0xEF or to 255,216,255,224 thru 239. You need to get comfortable working in hex, sometimes even binary!

Finally, there are two old sayings in programming.

1. KISS = Keep It Simple, Stupid!
2. Find the SECOND right answer!

KISS is meant to remind programmers to beware of overly complicated code! The more complex the code, the more likely that bugs will find their way into it. It's not meant to be an insult, I remind myself of it on every project. If you're doing something and your gut tells you that there is surely a simpler way to do something, there probably is. When it comes to simple tasks, almost everything that can be done has been done. There's probably a function to do it in a standard library somewhere. If you're doing something that seems trivial but is taking what seems like too many steps (initializing a string, or copying something into a string is a good example), look for a function! Google it! "How can I... in whatever language?" With ChatGPT, you could probably get a good answer and some code examples too. Learn these functions and get used to them. It'll speed up your programming time!

Another aspect of KISS is to NOT try to do too much at once. Elegant code will usually try to do one small task at a time as part of the big picture project. Code should be simple, it should do small tasks, and it should do those tasks separately from each other. Be very careful about trying to do things at the same time. THat's another way that logic bugs creep into code! If everything is all intertwined, it becomes nearly impossible to sort out what's doing what with what.

Find the SECOND right answer! This is a good one. There are lots of ways to do anything when coding. I've pointed out several examples above. Often, programmers will create code to do a task. They may spend a lot of time just to get it working. Once they get it working, some will be glad it's finished and move on. The good programmers will go back and look at the code again, usually after setting it aside for a few hours or even days! It given their minds time to digest all that work. Then, they come back to check the code and ask themselves, "Can this be done betteer?" That's the SECOND right way! The first working code works, but is it efficient? The second revision usually improves on the efficiency of the code. It may be no big deal on a homework assignment, but imagine the computing power saved by eliminating one extra fread or fwrite from a program's execution in a program that runs millions of times a day? (Or, perhaps, in a part of Twitter's source code that's used on every tweet read or write?) It's about being more efficient than that first pass.

That's a lot to digest, but I'm sure you'll see the value. Happy Coding! ;-)

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