0

I am getting the following output whenever I run my program:

Error in `./recover': double free or corruption (top): 0x0000000002a75250 Aborted

Also, when the program is executed, it does create the 50 .jpg files it should, but none are actual images. Instead, every file as a whole looks like a transparent background (i.e. grey and white pixels in a checkered pattern. I don't know if I'm explaining myself well enough...). This is my code:

// Recovers JPEG files from a forensic image

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

#define BLOCKOFBYTES 512

typedef uint8_t BYTE;

int main (int args, char* argv[])
{
    // ensure proper usage
    if (args != 2)
    {
        fprintf(stderr, "Usage: ./recover <forensic-image file>\n");
        return 1;
    }

    //open forensic image
    char* infile = argv[1];
    FILE* raw = fopen(infile, "r");
    if (raw == NULL)
    {
        fprintf(stderr, "Could not open %s\n", infile);
        return 2;
    }

    // declare buffer array
    BYTE buffer[BLOCKOFBYTES];

    // creates variables to keep track of JPEG names
    int track = 0;
    char filename[8];
    FILE *outfile = NULL;

    // iterate until the EOF
    while ((fread(buffer, BLOCKOFBYTES, 1, raw)) == 1)
    {
        // Checks bytes that identify a file as a JPEG
        if (buffer[0] == 0xff && buffer[1] == 0xd8 &&
        buffer [2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
        {
            //****** ARRIVED AT JPEG ******//

            sprintf(filename, "%03i.jpg", track);       // creates name of file
            outfile = fopen(filename, "w");             // opens new file with name
            track++;                                    // increments amount of JPEGs

            // Checks if file can be opened
            if (outfile == NULL)
            {
                fprintf(stderr, "Could not open %s\n", filename);
                fclose(outfile);
                return 3;
            }
            // writes 512 bytes to JPEG
            fwrite(buffer, BLOCKOFBYTES, 1, outfile);

            // Loops and writes so long as the first bytes of buffer array aren't JPEG identifiers
            while(buffer[0] != 0xff && buffer[1] != 0xd8 && buffer[2] != 0xff && (buffer[4] & 0xf0) == 0xe0)
            {
                fwrite(buffer, BLOCKOFBYTES, 1, outfile);
            }
            // closes previous JPEG
            fclose(outfile);
        }
        else
        {
        //****** NOT JPEG ******//
            // ALREADY FOUND JPEG?
            if (outfile == NULL)
            {
                // NO: discard 512 bytes, go back to start of loop.
                continue;
            }
            else
            {
                // YES: 512 bytes we read belong to current JPEG file
                fwrite(buffer, BLOCKOFBYTES, 1, outfile);
            }

        }

   }
   // closes remaining files
   fclose(outfile);

   // success
   return 0;
}

1 Answer 1

1

Most of the pieces are here, but this is a good example of overcomplicated code. Let's start with something simple. Look at this from the end of the code (comments removed):

        if (outfile == NULL)
        {
            continue;
        }
        else
        {
            fwrite(buffer, BLOCKOFBYTES, 1, outfile);
        }

When an if statement is followed by a code block that says "do nothing", like the continue statement does, and then followed by an else block that actually does something, this is a red flag that screams "simplify me!" It should be simplified by reversing the conditiion:

        if (outfile != NULL)
        {
            fwrite(buffer, BLOCKOFBYTES, 1, outfile);
        }

Reminder: An if statement is not required to have an else clause, and a NOT SOMETHING is a prefectly acceptable test.

Next, let's look at a while statement:

        // Loops and writes so long as the first bytes of buffer array
        //   aren't JPEG identifiers

        while(buffer[0] != 0xff && buffer[1] != 0xd8 && buffer[2] != 0xff
              && (buffer[4] & 0xf0) == 0xe0)
        { ...

This is NOT the reverse of the check for a signature. There's a long explanation for it, but it doesn't work by just flipping individual operators. In this case, if just one of the first 3 bytes matches, then it'll give a false positive result. If you want to easily reverse a complex logical test, just surround it with parentheses and put the NOT operator in front of it!

        while (   <complex test>   )    // a complex test
        while (  !(<complex test>) )    // tests for the reverse of complex test

Now for more complicated issues.

Did you notice that all of the files generated by your code had exactly 512 bytes? That's a good sign that only the first block of a file is being processed. Why? Not precisely sure. I opted to dig into the actual problems in the code logic instead. If you have the time, it would be a good exercise to track that down.

But, let's look at the code to process all the remaining blocks of data. Did you notice that you actually have two separate blocks of code to do that? Inside the main while loop, there's one inside the first IF statement code block and then another just following the end of that if block - in the else clause. One of these is redundant code.

On the good side, you appear to have recognized that there are two phases of code. First, check for a signature and open a file. Second, process the data block by either discarding or writing to an output file. But, when these two tasks were mixed in the first part, and one of them was in the second part, the code started getting too complicated. If I had to guess, I'd say that this was a middle-of-the-night-too-much-caffeine-not-enough-sleep coding session. ;-)

I'd suggest going back and rethinking the logic and restructure the code a bit, clean up some of the other stuff, and give some serious thought to what needs to be done and in what order.

BTW, when you find a signature, what should be done? Should a new file be opened? What about an already open output file?

There are a lot of hints and suggestions here. It should give you plenty to think about. Happy coding!

There's an old coder's axiom," KISS the code! ". In every coding project I've ever done, sooner or later I have to tell myself, "Keep It Simple, Stupid!" ;-)

If this answers your question(s), 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 .