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;
}