if condition is not executed (cs50 recover)

I've been trying to find the bug for several hours now. Writes a segmentation error. Help pls ;)

Task page: https://cs50.harvard.edu/x/2024/psets/4/recover

The assignment is to recover 50 JPEG files that were deleted from a memory card. We are instructed to check the first 4 bytes of each 512 byte pass of fread to indicate the start of a JPEG file and to write it to a new file. Once it reaches another 4 bytes that match, we are suppose to close the previous file and write to a new one until reaching EOF (the data from the memory card is provided to us in a file called card.raw).

In my decision if (buffer[0] == 0xff, etc. is not executed, after while immediately comes else and because, as i understand, a segmentation (core dumped) error is written.

That is, the code is compiled and after its execution the following is displayed: while executed else executed Segmentation fault (core dumped)

But the condition itself is written correctly, because I changed it to those written by other people. It looks like the data from the card is not being written to buffer.

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

#define BLOCK_SIZE 512
typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    // Accept a single command-line argument
    if (argc != 2)
    {
        printf("Usage: ./recover FILE\n");
        return 1;
    }

    // Open the memory card
    FILE *card = fopen(argv[1], "r");

    if (card == NULL)
    {
        printf("Could not open file.\n");
        return 1;
    }

    BYTE buffer[BLOCK_SIZE];
    int number_file = 0;
    char filename[8];
    FILE *img = NULL;
    bool this_first_jpeg = true;

    // While there's still data left to read from the memory card
    while(fread(buffer, BLOCK_SIZE, 1, card) == 1)
    {
        printf("while executed\n");
        // if start of new JPEG
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        {
            printf("if executed\n");
            if (this_first_jpeg == true)
            {
                sprintf(filename, "%03i.jpg", number_file++);
                img = fopen(filename, "w");
                fwrite(buffer, BLOCK_SIZE, 1, img);
                this_first_jpeg = false;
            }
            else
            {
                fclose(img);
                sprintf(filename, "%03i.jpg", number_file++);
                img = fopen(filename, "w");
                fwrite(buffer, BLOCK_SIZE, 1, img);
            }
        }

        // if already found JPEG
        else
        {
            printf("else executed\n");
            fwrite(buffer, 1, BLOCK_SIZE, img);
        }
    }

    fclose(card);
    fclose(img);

    return 0;
}

I tried to take pieces of other people's code and it still doesn't work. But the other code completely works.

Solution

There are multiple problems in your code:

you should open the memory card file in binary mode: FILE *card = fopen(argv[1], "rb");
same for the image files: img = fopen(filename, "wb");
if you do not have a JPEG signature (the else branch), you always write the block to img, which is NULL when you start the loop. Calling fwrite(buffer, 1, BLOCK_SIZE, img) with a null FILE pointer has undefined behavior, possibly a segmentation fault as observed.
after the loop, you always close img: fclose(img) will cause undefined behavior if img is NULL, ie if no image file was found. You must test if img is NULL.

Here is a modified version:

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

#define BLOCK_SIZE 512
typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    // Accept a single command-line argument
    if (argc != 2)
    {
        printf("Usage: ./recover FILE\n");
        return 1;
    }

    // Open the memory card
    FILE *card = fopen(argv[1], "rb");
    if (card == NULL)
    {
        fprintf(stderr, "Could not open file %s: %s\n", argv[1], strerror(errno));
        return 1;
    }

    BYTE buffer[BLOCK_SIZE];
    int number_file = 0;
    char filename[8];
    FILE *img = NULL;

    // While there's still data left to read from the memory card
    while (fread(buffer, BLOCK_SIZE, 1, card) == 1)
    {
        // if start of new JPEG
        if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff
        &&  (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        {
            if (img != NULL)
            {
                /* close current image */
                fclose(img);
            }
            snprintf(filename, sizeof filename, "%03i.jpg", number_file++);
            img = fopen(filename, "wb");
            if (img == NULL) {
                fprintf(stderr, "Could not open image file %s: %s\n",
                        filename, strerror(errno));
                fclose(card);
                return 1;
            }
        }
        if (img != NULL) {
            /* if current JPEG open, write the block */
            fwrite(buffer, BLOCK_SIZE, 1, img);
        }
    }

    fclose(card);
    if (img != NULL) {
        fclose(img);
    }

    return 0;
}