CS50 Problem set 4 Recover not recovering images

Question

I could do with some advice for this, to me this makes sense logically however when I run check50 only one of the images are recovered. I've looked through the code multiple times so I don't think its a syntax error so it must be some error with the logic. Any tips would be greatly appreciated.

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

typedef uint8_t BYTE;

bool is_jpeg_header(BYTE buffer[]);

int main(int argc, char *argv[])
{
    // Check if command line argument is valid
    if (argc != 2)
    {
        printf("Usage: ./recover image\n");
        return 1;
    }
    // Open memory card files
    char* mem_card = argv[1];
    FILE* inptr = fopen(mem_card, "r");
    if (inptr == NULL)
    {
        printf("File not found/n");
        return 1;
    }
    BYTE buffer[512];
    bool found_first_jpeg = false;
    int image_count = 0;
    char filename[8];
    FILE* outptr = NULL;

    while (!feof(inptr) && fread(buffer, sizeof(buffer), 1, inptr) == true)
    {
        // Check if we have found a JPEG
        if (is_jpeg_header(buffer) == true)
        {
            // Check if this is the first JPEG
            if (found_first_jpeg == false)
            {
                found_first_jpeg = true;
                sprintf(filename, "%03i.jpg", image_count);
                outptr = fopen(filename, "w");
                fwrite(buffer, sizeof(buffer), 1, outptr);
                image_count++;
            }
            // If this isn't the first JPEG, close file current JPEG and open new one for new JPEG
            else
            {
                fclose(outptr);
                image_count++;
                sprintf(filename, "%03i.jpg", image_count);
                outptr = fopen(filename, "w");
            }
        }
        // If we haven't found a new JPEG:
        else if (is_jpeg_header(buffer) == false)
        {
            // Continue reading file if we have not found first JPEG
            if (found_first_jpeg == false)
            {
                continue;
            }
            // Continue writing current JPEG into current file
            else
            {
                fwrite(buffer, sizeof(buffer), 1, outptr);
            }
        }
    }
    fclose(inptr);
    fclose(outptr);
    return 0;
}

bool is_jpeg_header(BYTE buffer[])
{
    if (((buffer[0] == 0xff) && (buffer [1] == 0xd8) && (buffer[2] == 0xff) && ((buffer[3] & 0xf0) == 0xe0)))
    {
        return true;
    }
    return false;
}

This is the error code I receive from check50

:) recover.c exists.
:) recover.c compiles.
:) handles lack of forensic image
:) recovers 000.jpg correctly
:( recovers middle images correctly
    001.jpg not found
:( recovers 049.jpg correctly
    recovered image does not match

Answer 1

One bug I see is that filename is too short: you haven't left any room for the terminating zero. That's undefined behavior, and likely your source of trouble.

But the logic overall is very convoluted for what amounts to a simple problem. Here's how I'd write it. Since you're not in general checking for errors, I've left it that way - it's likely OK for this test assignment, although I've not read it. It does help to return different error codes for different errors though - it'd have really helped with the original typo!

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

typedef uint8_t bool;
static const bool true = 1;
static const bool false = 0;

bool is_jpeg_header(const uint8_t buffer[]);

int main(int argc, char *argv[])
{
    // Check if command line argument is valid
    if (argc != 2)
    {
        printf("Usage: ./recover image\n");
        return 1;
    }

    // Open the memory card image
    char* mem_card = argv[1];
    FILE* infile = fopen(mem_card, "r");
    if (!infile)
    {
        printf("File not found/n");
        return 2;
    }

    uint8_t buffer[512];
    int image_count = 0;
    char filename[9];
    FILE* outfile = NULL;

    while (!feof(infile) && fread(buffer, sizeof(buffer), 1, infile) == 1)
    {
        // Check if we have found a JPEG
        if (is_jpeg_header(buffer))
        {
            // If we're already writing output - close it
            if (outfile)
                fclose(outfile);

            sprintf(filename, "%03i.jpg", image_count);
            outfile = fopen(filename, "w");
            image_count ++;
        }

        // Write the output if we're ready to write
        if (outfile)
            fwrite(buffer, sizeof(buffer), 1, outfile);
    }
    fclose(infile);
    fclose(outfile);
    return 0;
}

bool is_jpeg_header(const uint8_t buffer[])
{
    return
        buffer[0] == 0xff
        && buffer[1] == 0xd8
        && buffer[2] == 0xff
        && (buffer[3] & 0xf0) == 0xe0;
}

Answer 2

why while(!foef() is always wrong

regarding:

printf("File not found/n");

Error messages should be output to stderr, not stdout.

When the error indication comes from a C library function should also output to stderr, the text reason the system thinks the error occurred. The function: perror( "your error msg" );

is made for this purpose.

regarding:

printf("Usage: ./recover image\n"); 

1) this should be to stderr, not stdout. 2) don't hardcode the executable name. Suggest:

fprintf( stderr, "Usage: %s imagefile\n". argv[0] );

regarding:

while (!feof(inptr) && fread(buffer, sizeof(buffer), 1, inptr) == true) 

1) true and false are defined in stdbool.h so that header file needs to be included.

2) fread() returns the number of items read. (which is also the third parameter, (and remembering the prior statement about while( !foef() ) so the statement would be much better written as:

while (  fread(buffer, sizeof(buffer), 1, inptr) == 1) 

which catches EOF and partial reads and I/O errors.

regarding;

outptr = fopen(filename, "w"); 

The success/failure of the call to fopen() is not under the control of your program, Therefore, always check (!=NULL) the returned value to assure the operation was successful.

regarding:

if (is_jpeg_header(buffer) == false)         
{             // Continue reading file if we have not found first JPEG 
    if (found_first_jpeg == false)             
    {                 
        continue;             
    }             // Continue writing current JPEG into current file  

    else 

This code can be completely removed

the posted code fails to close the current output file after the second file is started.

the posted code always reads sizeof(buffer) bytes (assuming no errors) but there is no guarantee that each image data is exactly a multiple of sizeof(buffer) in length so it can miss the encounter with the next image AND can result in part of the header, etc data from the next image being written into the current output file.

Please post the function:

is_jpeg_header(buffer) 

as it is unlikely to correct the problems listed above.