pset4 recover.c Why are my pictures scrambled?

Question

My attempt at recover.c almost works. It creates 50 jpegs and names them appropriately. And it fills them part way before getting scrambled. Here is an example:

Here is my code:

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

int main(int argc, string argv[])
{
//Ensure proper usage
if (argc != 2)
{
    printf("Usage: ./recover file\n");
    return 1;
}

//Remember filename.
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;
}

//Setting the buffer to hold 512 bytes.
uint8_t buffer[512];
char filename[50];
int counter = 0;
FILE *outptr;

for (int b = 0; b < 23107; b++)
{
    fread(buffer, 512, 1, inptr);

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

//This is performs a linear search.
    if (buffer[0] == 0xff && 
        buffer[1] == 0xd8 &&
        buffer[2] == 0xff &&
        (buffer[3] & 0xf0) == 0xe0)
    {
        if (counter != 0)
        {
            fclose(outptr);
        }
    //Opening and naming the outfile.
    sprintf(filename, "%03i.jpg", counter);
    outptr = fopen(filename, "w");
    counter++;
    fwrite(buffer, 512, 1, outptr);
    }
   }

    fclose(inptr);

    return 0;
}

Now, what I don’t understand is, why do the pictures go wonky partway through?

As far as I can tell, the code should keep writing the 512 bytes in the buffer into the outfile. I have run debug50, keeping an eye on the loop past the point where the image gets corrupted, and it seems to keep cycling through as it’s supposed to.

I’m not particularly confident with the lines of code that close each jpeg, but that’s a fairly late addition and does not seemed to have had any impact, one way or the other.

Thanks for reading.

Answer 1

De Morgan's laws

The opposite of

    buffer[0] == 0xff && 
    buffer[1] == 0xd8 &&
    buffer[2] == 0xff &&
    (buffer[3] & 0xf0) == 0xe0

is

    buffer[0] != 0xff ||
    buffer[1] != 0xd8 ||
    buffer[2] != 0xff ||
    (buffer[3] & 0xf0) != 0xe0

Notice how !(A && B) is !A || !B, with && turning into || (similarly, !(A || B) is !A && !B).

You might need to add some parentheses, as && has higher precedence than ||, like

    if (counter > 0 &&
        (buffer[0] != 0xff || 
         buffer[1] != 0xd8 ||
         buffer[2] != 0xff ||
         (buffer[3] & 0xf0) != 0xe0))
    {
        fwrite(buffer, 512, 1, outptr);
    }

Another way is to change the structure of your loop. Move the write below your new file detection, remove the fwrite from the file detection part, have only if (counter > 0) for the remaining write. In my opinion, this would be the cleaner solution, with much less code.