CS50 week 4 recover, problem with loading 000.jpg, every other image is fine

Question

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

//Creates a new variable BYTE that stores a BYTE of data
typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    //Incorrect Number of CLA
    if (argc != 2){
        printf("Improper Usage\n");
        return 1;
    }
    //Incorrect Files Inputted
    if (argv[1] == NULL){
        printf("Usage: ./recover IMAGE\n");
    }

    //initializing the buffer
    BYTE buffer[512];

    //Open the Memory Card and Initialize the file pointer pointing towards it ----- fread
    FILE *f = fopen(argv[1], "r");

    //variable for filename
    int count = 0;

    //create a new block of memory to store the filename
    char *filename = malloc(8);

    //Set the name of the string to later name the file
    sprintf(filename, "%03i.jpg", count);

    //creating a new file with the name set
    FILE *new_file = fopen(filename, "w");

    if (new_file == NULL) {
        fprintf(stderr, "Failed to open file: %s\n", filename);
        free(filename);
        fclose(f);
        return 1;
    }

    //while loop that runs till the end of the file
    while (fread(buffer, 1, 512, f) == 512){
        //Look for the beginning of a JPEG file ( 0xff, 0xd8, 0xff, 0xe0) ----- &0xf0
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0){
            if (count == 0){
                //writing the block of 512 bytes to the new file
                fwrite(buffer, 1, 512, new_file);

                //increase the count according to the number of file
                count++;
            } else {
                fclose(new_file);
                sprintf(filename, "%03i.jpg", count);
                count++;
                new_file = fopen(filename, "w");
                fwrite(buffer, 1, 512, new_file);
            }
        }else{
            //writing the data contiguosly stored in the memory card
            fwrite(buffer, 1, 512, new_file);
        }
    }
    //free
    fclose(f);
    fclose(new_file);
    free(filename);
}

Since the problem is arising with the first jpeg 000.jpg, I'm guessing it has got something to do with the if (count == 0) part. But can seem to figure it out. I have tried to initialize new_file outside the while loop and opening it in the if cond. but it not working.

Please tell me what could be the problem.

Another recent and similar cs50 question [here](https://stackoverflow.com/q/76683425/2410359). — chux - Reinstate Monica, Jul 14 '23 at 07:44
OT: If `argc == 2` then `argv[1]` by definition can't be a `NULL` pointer. Your `argv` check is redundant and not needed. — Some programmer dude, Jul 14 '23 at 07:45
Why ignore the return value of `fopen()` and `malloc()`? These functions return `NULL` to indicate failure. — Harith, Jul 14 '23 at 07:45
OT: Why do you allocate memory for the filename dynamically? Why not use an array? — Some programmer dude, Jul 14 '23 at 07:45
Lastly, it's time to learn how to [*debug*](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/) your programs. Use a [*debugger*](https://stackoverflow.com/questions/25385173/what-is-a-debugger-and-how-can-it-help-me-diagnose-problems) to step through your code line by line while monitoring your variables and their values, to see what actually happens. — Some programmer dude, Jul 14 '23 at 07:47
Have you tried running your code line-by-line in a debugger while monitoring the control flow and the values of all variables, in order to determine in which line your program stops behaving as intended? In week 2 of CS50, you were taught how to use a debugger. Note that [the lesson of week 2 has a chapter named "Debugging"](https://video.cs50.io/XmYnsO7iSI8?screen=5YGV1hcM_MY&start=2294&end=3777). Week 2 also has a few short videos on debugging. — Andreas Wenzel, Jul 14 '23 at 11:40

chux - Reinstate Monica · Answer 1 · 2023-07-14T08:24:34.933

Code has at least these problems:

Binary mode

.jpg files should be opened in binary mode

// fopen(filename, "w");
fopen(filename, "wb");

Buffer overflow risk

When count > 999, buffer is too small.

// char *filename = malloc(8);
char *filename = malloc(15);
sprintf(filename, "%03i.jpg", count);

Also research snprintf().

What if the file length is not a multiple of 512?

OP's code does not well handle such a case.

//while loop that runs till the end of the file
while (fread(buffer, 1, 512, f) == 512){

Unclear what OP's goals are here.

Check for errors

Check the return value of fopen(), malloc(), fwrite(). @Harris.

Design flaw

Code is strange in that if the first block read does not meet the buffer[0] == 0xff && buffer[1] == 0xd8 ..., that it is written to "000.jpg" anyways. Perhaps it should be tossed?

CS50 week 4 recover, problem with loading 000.jpg, every other image is fine

1 Answers1