Not encountering end of file (eof) in C

Question

I am trying to run the code. All the images are coming fine as per specification excluding the last one.

The first four bytes (B) repeating are as follows :

b8 97 98 c5

The end of file is not encountered as a result the last image is found corrupted.

EDIT:

1. It is already mentioned that there are 50 images in the file.
2. You can get the raw file from : http://cdn.cs50.net/2017/fall/psets/4/recover/card.raw

The original code is as follows :

// Recovers lost images (.jpeg) in a memory card

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

#define buffsize 10

// Function to check whether jpeg or not
int check_jpeg(unsigned char *argv) {
    unsigned int v1 = (int)argv[0];
    unsigned int v2 = (int)argv[1];
    unsigned int v3 = (int)argv[2];
    unsigned int v4 = (int)argv[3];
    if (v1 == 0xff && v2 == 0xd8 && v3 == 0xff) {
        switch (v4) {
          case 0xe0:
          case 0xe1:
          case 0xe2:
          case 0xe3:
          case 0xe4:
          case 0xe5:
          case 0xe6:
          case 0xe7:
          case 0xe9:
          case 0xea:
          case 0xeb:
          case 0xec:
          case 0xed:
          case 0xee:
          case 0xef:
            return 1;
            break;
          default:
            return 0;
        }
    } else {
        return 0;
    }
}

int main(int argc, char *argv[]) {
    // Cautioning the user for wrong usage
    if (argc != 2) {
        fprintf(stderr, "Usage: ./recover file\n");
        return 1;
    }

    // Opens the .raw file to begin inspection
    FILE *camera = fopen(argv[1], "r");

    // Checks the validity of the opened file
    if (camera == NULL) {
        fprintf(stderr, "Error opening file: %s\n",argv[1]);
        return 2;
    }

    int counter = 0; // Declaring and Initialising the counter
    int online = 0; // To know whether image is being written

    char *filename = (char*)malloc(buffsize);
    FILE *outptr;

    while (1) {
        unsigned char *image = malloc(512);
        if (image == NULL) {
            fprintf(stderr, "Error creating pointer \n");
            return 200;
        }
        fread(image, 512, 1, camera);
        if (image != NULL) {
            int flag = check_jpeg(image);
            if (counter == 50) {
                printf("%x %x %x %x\n", image[0], image[1], image[2], image[3]);
            }
            if (flag == 1) {
                if (counter != 0) {
                    fclose(outptr);
                }
                counter++;

                // Creating the output file pointer
                snprintf(filename, buffsize - 1, "%03i.jpg", counter);
                outptr = fopen(filename, "w");
                if (outptr == NULL) {
                    fprintf(stderr, "Error opening file: %s\n", filename);
                    return 201;
                }

                // Writing to the file
                fwrite(image, 512, 1, outptr);
                online = 1;
            } else
            if (flag == 0 && online == 1) {
                fwrite(image, 512, 1, outptr); // Continue writing to the output file
            }
            free(image);
        } else {
            fclose(camera);
            fclose(outptr);
            return 0;
        }
    }
}

Answer 1

There are multiple issues in your code:

• you do not check how much data fread successfully reads. At end of file, fread returns 0, otherwise fread returns the number of blocks successfully read into the destination array. To keep track of bytes read, pass 1 as the block size and 512 as the number of blocks.
• there is no real need to allocate the filename and the input/output buffers, local arrays are fine for your purpose.
• the files should be open in binary mode: FILE *camera = fopen(argv[1], "rb");
• the second argument to snprintf should be the buffer size, not the maximum number of characters to write: snprintf(filename, buffsize, "%03i.jpg", counter);

Answer 2

fread wont set the pointer (or is at least not guaranteed to) to NULL on failed read. In fact I think it is supposed to leave the pointer unchanged. fread will however return the number of bytes read, so you could change:

   fread(image, 512, 1, camera);
   if (image != NULL)

to

   int bytesread=fread(image, 1, 512, camera);
   if (bytesread!= 512)

Answer 3

You might be tempted to do the following:

while (!feof(camera)) {

However, this works only in the case of there being no other errors reading the file, and even then always results in there being one additional read of the file (the one that triggers the EOF condition). That last read may return bad or point to stale data and so needs to be handled as per @chqrlie's answer and this previous question about feof().

Bottom line: Check the number of bytes read, if it less than requested then use ferror() and feof() to isolate the cause so you can respond accordingly.