Recovering JPEG image from .raw file using C

Question

I am doing a problem set provided by Harvard's online lecture. I've finally made a solution to recover a set of JPEG images from a file (card.raw).

It seems like the code itself does not throw errors, but it is returning distorted image and I am a little clueless to why it might be happening.

[Link to an image example] https://prnt.sc/q0tb4f

Here's my code

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


int main(int argc, char *argv[])
{
    //check usage
    if (argc != 2)
    {
        return 1;
    }
    // open file
    FILE* input = fopen(argv[1], "r");
    // return error if file does not existS
    if (!input)
    {
        return 1;
        printf("file does not exists");
    }
    // create an array with 512 bytess of bytes
    unsigned char bytes[512];
    // create count variable
    int count = 0;
    // create an empty string for filename
    char filename[7];
    // create an empty output file
    FILE* output = NULL;
    // repeat until end of input
    while (fread(bytes, 1, sizeof(bytes), input) != 0)
    {
        // read 1 block of 512 bytes at a time
        fread(bytes, 1, sizeof(bytes), input);

        // check if beginning of jpeg file
        if (bytes[0] == 0xff && bytes[1] == 0xd8 && bytes[2] == 0xff && (bytes[3] & 0xf0) == 0xe0)
        {
            // if already found a jpeg, close the file
            if (count > 0)
            {
                fclose(output);
            }
            // name file
            sprintf(filename, "%03i.jpg", count);
            // open file
            output = fopen(filename, "w");
            // write file
            fwrite(bytes, 1, sizeof(bytes), output);
            // increment count
            count++;
        }
        if (output != NULL)
        {
            // keep writing if jpeg header is already found
            fwrite(bytes, 1, sizeof(bytes), output);
        }
    }
     fclose(output);
     fclose(input);
}

My uneducated assumption is unable to see why it might be happening. I can only imagine that this might be happening from opening and closing files in improper step.

Answer 1

here is recover working.

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

int isAJpg(unsigned char bytes[]);

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        return 1;
    }
    FILE *file = fopen(argv[1], "r");
    if (file == NULL)
    {
        return 1;
    }
   
    char filename[10];
    int count = 0;
    unsigned char bytes[512];
    FILE *output;
    int jpgfound = 0;
   
    while (fread(bytes, 512, 1, file) != 0)
    {
       
        // if it is a jpg
        if (isAJpg(bytes) == 1)
        {
            if (jpgfound == 1)
            {
                fclose(output);
            }
            else
            {
                jpgfound = 1;
            }
            // name file
            sprintf(filename, "%03d.jpg", count);
           
            // open file
            output = fopen(filename, "a");
           
            count++;
        }
        if (jpgfound == 1)
        {
            // writes to a file
            fwrite(&bytes, 512, 1, output);
        }
    }
    //close the files
    fclose(output);
    fclose(file);
}

// check in it is a jpg
int isAJpg(unsigned char bytes[])
{
    if (bytes[0] == 0xff && bytes[1] == 0xd8 && bytes[2] == 0xff && (bytes[3] & 0xf0) == 0xe0)
    {
        return 1;
    }
    return 0;
}

Answer 2

This is the problem:

while (fread(bytes, 1, sizeof(bytes), input) != 0)
{
    // read 1 block of 512 bytes at a time
    fread(bytes, 1, sizeof(bytes), input);

You're calling fread twice per loop. As a result, the body of the loop only sees the odd-numbered blocks. Remove the second fread.

A second problem (as @SteveFriedl points out) is that the buffer the code uses for the filename is too small.

char filename[7];
sprintf(filename, "%03i.jpg", count);

You need at least 8 bytes for a file name like "123.jpg", because you need room for the NUL terminator. However, note that "%03i" uses at least 3 characters. It could use more, e.g. if count reaches 1000. So I would declare the buffer as char filename[32]; to avoid any chance of buffer overflow.

You've also got two fwrites when only one is needed:

        output = fopen(filename, "w");
        // write file
        fwrite(bytes, 1, sizeof(bytes), output);
        // increment count
        count++;
    }
    if (output != NULL)
    {
        // keep writing if jpeg header is already found
        fwrite(bytes, 1, sizeof(bytes), output);
    }

After opening a new file, the code writes the first block, and then writes it again. Remove the first fwrite and let the second fwrite take care of the first block.

Another problem is that the code makes an implicit assumption that if fread doesn't return 0, then it's read a full block. That assumption is OK if the file size is a multiple of the block size, but it's better not to make any assumptions. So the condition in the while loop should be

while (fread(bytes, 1, sizeof(bytes), input) == sizeof(bytes))