Recover cs50: It recovered 000.jpg but when i run it again without making any changes with check50. It didnt work?

Question

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


int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        printf("./recover file_name\n");
        return 1;
    }
    
    FILE *file = fopen(argv[1],"r");

    if (file == NULL)
    {
        printf("This file cannot be opened.\n");
        return 1;
    }
    
    //512B is the size of 1 block
    unsigned char buffer[512];
    FILE *img = NULL;
    
    //we alr know that there is 50 images for us to recover
    string filename[50];
    int count = 0;
    
    // read 1 element of 512B into buffer array
    while (fread(buffer, sizeof(unsigned char), 512, file) == 512 *sizeof(unsigned char))
    {
        //NEW jpeg
        if (img == NULL)
        {
            //start of jpeg
            if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
            {
                //create filename(string not array) for new file
                sprintf(filename[count], "%03i.jpg", count);
                
                //create new jpeg file with new filename & write/append in card.raw
                img = fopen(filename[count],"w");
                fwrite(buffer, 512, 1, img);
                fclose(img);
            }
            //continue while loop and read file if cannot find jpeg signature
        }
        else
        {
            //Discover jpeg signature
            //End of jpeg + Start of new jpeg
            if (buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] & 0xf0) == 0xe0)
            {
                fclose(img);
                //intialize pointer
                img = NULL;
                count++;
                
                //create filename(string not array) for new file
                sprintf(filename[count], "%03i.jpg", count);
                
                //create new jpeg file with new filename & write/append in card.raw
                img = fopen(filename[count],"w");
                fwrite(buffer, 512, 1, img);
            }
            else
            {
                //continue writing to jpeg file
                fwrite(buffer, 512, 1, img);
            }
        }
    }
    
    if (!feof(file))
    {
        printf("Unknown error occured\n");
        return 1;
    }
    
    printf("%i jpeg images recovered\n", count);
    fclose(file);
    return 0;
}

UndefinedBehaviourSanitizer: DEADLYSIGNAL The signal is caused by WRITE memory access Hint:address points to zero page

What's the problem? I can't seem to figure it out. Is it a logic problem? It recovered 000.jpg but when i run it again without making any changes with check50(at least I don't think I made any changes). It didnt work??

Answer 1

The thing is: one should only use the string datatype if it will be populated by get_string. I don't think it is explicitly mentioned in the course material, but because of memory allocation, it will not work otherwise. Since the 50 elements of filename do not get properly allocated, there will be unpredictable results.

Consider this: Does filename need to be an array of strings? Is there any reason to "keep" all the file names? Each will be used exactly once, and not needed again. If filename was an array of chars, it would suffice. It would have to be declared with enough bytes to accomodate a file name and the terminating null byte.

While this //we alr know that there is 50 images for us to recover is true for the raw file contained in the distro code, it is unnecessarily limiting. What if the grading program used a raw file that only contained 10 photos, or contained 100 photos?