My recover file from cs50x pset4 only recovers the corner of the files

Question

I was trying to solve the problem recover of pset 4 of cs50x. We are supposed to create a new image whenever we discover the correct header of a jgp file, which i tried to do so. But for some reason, the code still only writes the images partially. Whenever the code discovers the correct header, it increases the value of img_count and only then opens a new image.

Link to the problem: https://cs50.harvard.edu/x/2024/psets/4/recover/

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

typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    int j = 0;
    if (argc != 2)
    {
        printf("Usage: ./recover forensic_image_file\n");
        return 1;
    }
    FILE *for_image = fopen(argv[1], "r");
    if (for_image == NULL)
    {
        printf("Could not open %s.\n", argv[1]);
        return 1;
    }
    char img_recovered[8];
    img_recovered[3] = '.';
    img_recovered[4] = 'j';
    img_recovered[5] = 'p';
    img_recovered[6] = 'g';
    int img_count = -1;
    BYTE buffer[512];
    while (fread(buffer, 1, 512, for_image))
    {
        if ((buffer[0] = 255) && (buffer[1] == 216) && (buffer[2] == 255) && (buffer[3] / 16 == 14))
        {
            j++;
            img_count++;
            img_recovered[0] = (img_count / 100) + '0';
            img_recovered[1] = ((img_count % 100) / 10) + '0';
            img_recovered[2] = (img_count % 10) + '0';
            FILE *for_recovery = fopen(img_recovered, "w");
            fwrite(buffer, 1, 512, for_recovery);
            fclose(for_recovery);
        }
        else if (j > 0)
        {
            img_recovered[0] = (img_count / 100) + '0';
            img_recovered[1] = ((img_count % 100) / 10) + '0';
            img_recovered[2] = (img_count % 10) + '0';
            FILE *for_recovery = fopen(img_recovered, "a");
            fwrite(buffer, 1, 512, for_recovery);
            fclose(for_recovery);
        }
    }
    fclose(for_image);
}

Answer 1

You have a bug.

In your if statement inside your loop, you used an assignment operator (=) in place of a comparison operator (==)

Change:

(buffer[0] = 255)

Into:

(buffer[0] == 255)

With the bug, the code was changing every 512th byte into 0xFF

---

Also, img_recovered is uninitialized, so you have undefined behavior:

Change:

char img_recovered[8];

Into:

char img_recovered[8] = { 0 };

---

Your code has a bit of replication. It could be cleaned up, simplified, and made faster by not doing fopen/fclose for each output block:

Here is the refactored code:

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

typedef uint8_t BYTE;

int
main(int argc, char *argv[])
{
    int j = 0;

    if (argc != 2) {
        printf("Usage: ./recover forensic_image_file\n");
        return 1;
    }
    FILE *for_image = fopen(argv[1], "r");

    if (for_image == NULL) {
        printf("Could not open %s.\n", argv[1]);
        return 1;
    }
    char img_recovered[8] = { 0 };

    img_recovered[3] = '.';
    img_recovered[4] = 'j';
    img_recovered[5] = 'p';
    img_recovered[6] = 'g';
    int img_count = -1;
    FILE *for_recovery = NULL;
    BYTE buffer[512];

    while (fread(buffer, 1, 512, for_image)) {
        if ((buffer[0] == 255) && (buffer[1] == 216) && (buffer[2] == 255) && (buffer[3] / 16 == 14)) {
            img_count++;

            if (for_recovery != NULL)
                fclose(for_recovery);

            img_recovered[0] = (img_count / 100) + '0';
            img_recovered[1] = ((img_count % 100) / 10) + '0';
            img_recovered[2] = (img_count % 10) + '0';

            for_recovery = fopen(img_recovered, "w");
        }

        if (for_recovery != NULL)
            fwrite(buffer, 1, 512, for_recovery);
    }

    if (for_recovery != NULL)
        fclose(for_recovery);

    fclose(for_image);
}