Fread doesn't return new values to buffer - recover - pset4

Question

I am currently working with recover and when I first compiled my program with debug50, my program was able to return 6 corrupted files without a propre name. When I wanted to get a closer look as to why this happened, it turns out buffer keeps the same first 4 bytes throughout the whole program. After wanting to test my program 1 more time, thevalues ofthe first 4 bytes changed and stayed that way no matter how many times I recompiled and ran my program. Any ideas as to why this happens?

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

typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    //checking if a file is given
    if(argc != 2)
    {
        printf("Usage: ./recover image\n");
        return 1;
    }
    char *image = argv[1];
    FILE *JPG = fopen(image, "r");
    //checking if file could be opened
    if (JPG == NULL)
    {
        printf("Could not open %s.\n", image);
        return 2;
    }
    //declaring the variables
    int n = 000;
    FILE* img = NULL;
    int x = 512;
    BYTE buffer[512];

    while (1 == 1)
    {
        //detecting the start of a new JPEG
        fread(buffer, 1, 512, JPG);
        int a = buffer[0];
        int b = buffer[1];
        int c = buffer[2];
        int d = buffer[3];
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
        {
            char filename[8];
            sprintf(filename, "%03i.jpg", n);
            img = fopen(image, "w");
            while (1 == 1)
            {
                //writing until new file is found
                fread(buffer, 1, 512, JPG);
                x = fread(buffer, 1, 512, JPG);
                a = buffer[0];
                b = buffer[1];
                c = buffer[2];
                d = buffer[3];
                if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
                {
                    break;
                }
                //ending when end of file reached
                if (x != 512)
                {
                    return 0;
                }
                fwrite(buffer, 1, 512, img);
            }
            //closing the file
            fclose(img);
            n++;
        }
    }
}

Answer 1

One major fix is to change this line:

        img = fopen(image, "w");

It's opening the input file for write! That will erase the current contents of the input file and just cause all kinds of problems!

Next, the code has too many fread statements. In one place, there are two consecutive fread statements, which will discard every other data block without processing it.

There is also dead code (code that does nothing or serves no purpose) that needs to be removed. Specifically, remove all the declarations/assignments of vars a,b,c,d.

Next, creating a while(true) loop when there's an obvious test for exit is a bad practice. It's often an invitation to creating an infinite loop. In this case, the fread can be incorporated into the while loop setup and the value returned from the fread call can be used to decide whether to terminate the loop. It would also eliminate the need for code inside the while loop to exit the code. Nesting two forever loops is definitely not a good idea in general. (There are specific times when a while(true) loop is a good idea, such as when the intent is to create a forever loop - commonly used when creating a code daemon, but not a good practice in general.)

My advice is to go back and rethink the entire while block. HINT: When code is duplicated nearly exactly in a program, it should be a red flag that the code is redundant and that section can probably be rewritten more efficiently (or a function should be written). It's possible to create an efficient version of this code with a single fread and a single fwrite call in a single while loop.