Seg fault after comparison `(card_r[0] == 0xff)` of bytes in Recover pset4, and question about fread in a while loop

Question

I have a question concerning:

1. The part while (fread(card_r, sizeof(int), 512, inptr) == 512) I suppose I am reading into card_r here and don't need an additional fread statement for reading. I have tried with another fread statement but I noticed I was getting same values in debugger50 no matter which one I used. Cannot test again since debugger hasn't worked for me for a couple hours now...

2. The seg fault, to my understanding I am comparing the (card_r[0] == 0xff), if(card_r[1] == 0xd8), if(card_r[2] == 0xff) and ((card_r[3] & 0xf0) == 0xe0) respectively. card_r should have enough space for 512bytes or `512 * sizeof(int). So I don't see why I get seg fault in that part, perhaps I'm managing my memory wrong with the pointer?

3. Debugger50 stopped working after 5 hours of debugging, I guess this doesn't belong here but woulde be nice to get it working again :). I've tried to closing browser and refresh but as it ./recover card.raw the program doesn't run but compiles ok.

Thanks for the help and I hope I've formatted my question properly and sorry for the messy code, I had to reshape some lines for debugging.

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

int main(int argc, char *argv[])
{
// ensure proper usage
    if (argc != 2)
    {
        fprintf(stderr, "Usage: ./recover Name of the forensic image\n");
        return 1;
    }

    FILE *inptr = fopen(argv[1], "r"); // remember filename

    if (inptr == NULL)
    {
        fclose(inptr);
        fprintf(stderr, "Cannot open %s\n", argv[1]);
        return 2;
    }

    FILE *img = NULL; // file pointer for storing new jpegs
    int *card_r = (int *) malloc(512 * sizeof(int)); // buffer for reading from inptr
    /* above I've tried both without  * sizeof(int) and with it */
    char images[strlen("###.jpg") + 1]; //array for storing filenames
    int found = 000; //track found jpegs

    while (fread(card_r, sizeof(int), 512, inptr) == 512)
    /* above tried both with sizeof(int) and 1  as the 2nd parameter*/
    {
        //fread(card_r, 1, 512, inptr); 
        // have we found a jpeg? JPEG first four bytes 0xff 0xd8 0xff (0xf0 - 0xe0)
        /* ORIGINAL if (card_r[0] == 0xff && card_r[1] == 0xd8 && card_r[2] == 0xff && (card_r[3] & 0xf0) == 0xe0) */
        if (card_r[0] == 0xff)
        {
   printf("success"); /*Segf after the going through comparison. I separated the comparisons for debugging, always seg fault after the comparison.*/
            if(card_r[1] == 0xd8)
            {
                printf("success");   // FOR DEBUGGING
                if(card_r[2] == 0xff)
                {
                    printf("success");
                    if ((card_r[3] & 0xf0) == 0xe0)
                    {
                        printf("success");
                    }
                }
            }
            {
                found++;
                // check if we have already found a jpeg?
                if (img == NULL)  // no
                {
                    sprintf(images, "%03i", found);
                    img = fopen(images, "w");
                    fwrite(card_r, 512, sizeof(int), img);
                }
                else
                { // yes
                    fclose(img);
                    sprintf(images, "%03i", found);
                    img = fopen(images, "w");
                    fwrite(card_r, 512, 1, img);
                }
            }
        }
            else // check if we have already found a jpeg?
            {
                if (img != NULL) // no
                {
                    fwrite(card_r, 512, 1, img);
                }
            }
    }

// success
fclose(inptr);
fclose(img);
free(card_r);
return 0;
}`

Answer 1

1. You should read 512 bytes, not 512 ints. A type holding a byte could be the BYTE alias, or unsigned char, or, my preference, uint8_t.

   And yes, a single fread in the loop head should be sufficient.

2. You use malloc, but never check its return value. Please don't do that (although it's unlikely for malloc to ever return NULL, especially in this setting). You could use an array on stack instead and not deal with memory allocation manually.

3. Your current code probably never opened some output image, so the final fclose(img) fails with a segmentation fault. Use something like if (img) { fclose(img); }