Reading jpegs from a SD card using C

Question

For my C class, I have to write a program that looks through an SD card, or really a card.raw file, and pull out the images that were accidentally deleted. The issue that I am having is that I keep getting a segmentation fault and I do not know why. I think it may be that I am not using data pointer correctly, but I am unaware of how else to do it. I will have some code here so thoughts are appreciated! The part that should be looked at is the recover function as that it the piece that is having the trouble, teacher pretty much gave us the rest of the code.

#include <stdio.h>
#include <stdlib.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>

// Function prototypes. Don't change these.
unsigned char * read_card(char * fname, int *size);
void save_jpeg(unsigned char * data, int size, char * fname);
void recover(unsigned char * data, int size);

int main()
{
    // Read the card.raw file into an array of bytes (unsigned char)
    int card_length;
    unsigned char *card = read_card("card.raw", &card_length);

    // Recover the images
    recover(card, card_length);
}

unsigned char * read_card(char * fname, int * size)
{

    struct stat st;
    if (stat(fname, &st) == -1)
    {
        fprintf(stderr, "Can't get info about %s\n", fname);
        exit(1);
    }
    int len = st.st_size;
    unsigned char *raw = (unsigned char *)malloc(len * sizeof(unsigned 
char));

    FILE *fp = fopen(fname, "rb");
    if (!fp)
    {
        fprintf(stderr, "Can't open %s for reading\n", fname);
        exit(1);
    }

    char buf[512];
    int r = 0;
    while (fread(buf, 1, 512, fp))
    {
        for (int i = 0; i < 512; i++)
    {
        raw[r] = buf[i];
        r++;
    }
    }
    fclose(fp);

    *size = len;
    return raw;
}

void save_jpeg(unsigned char * data, int size, char * fname)
{
    FILE *fp = fopen(fname, "wb");
    if (!fp)
    {
        fprintf(stderr, "Can't write to %s\n", fname);
        exit(1);
    }

    fwrite(data, 1, size, fp);
    fclose(fp);   
}
void recover(unsigned char * data, int size){
    int start=0;
    int length=0;
    int count=0;
    char name[64];

    for( int i = 0; i < size ;i++ ){
        if ((data[i] == 0xff && data[i+1] == 0xd8 && data[i+2] == 0xff && 
data[i+3] == 0xe0) ||
            (data[i] == 0xff && data[i+1] == 0xd8 && data[i+2] == 0xff && 
rmdata[i+3] == 0xe1)){
               start = i;

            }
        if(data[i] == 0xd9){
            length = i - start;
            count++;
            sprintf(name,"pic%d.jpeg",count);
            save_jpeg(data[start], length, name);

        }


    }

}

Answer 1

The way you are reading is a bit wrong, it would only work if the file size is a multiple of 512. You have to get the number of bytes fread read and write only that amount. Because you are reading blocks of 512 bytes, the last block might not be that large, hence you might overflow the buffer which leads to a segfault.

Change you reading to:

unsigned char buf[512];
size_t n;
while ((n = fread(buf, 1, sizeof buf / sizeof buf[0], fp)))
{
    for (int i = 0; i < n; i++) // the i < n is the correct way
    {
        raw[r] = buf[i];
        r++;
    }
}

Also it is bad practice to do exit(1) when a function fails, it's better that this function should return NULL instead and the calling function should check if the returned value is NULL:

unsigned char * read_card(char * fname, int * size)
{

    struct stat st;
    if (stat(fname, &st) == -1)
    {
        fprintf(stderr, "Can't get info about %s\n", fname);
        return NULL; // <-- here do not do exit
    }
    ...

}

int main(void)
{
    int card_length;
    unsigned char *card = read_card("card.raw", &card_length);

    if(card == NULL)
    {
        fprintf(stder, "Error, could not read card.raw\n");
        return 1;
    }

    recover(card, card_length);

    // do not forget to free the memory
    free(card);

    return 0;
}

Also do not cast malloc, do it like this:

unsigned char *raw = malloc(len * sizeof *raw);

and because the size of a char is defined as 1, in this case you can even remove the * sizeof *raw.

I don't know how the jpeg format works, so I cannot comment on whether your recover is doing the correct thing. But there is one thing you should think about:

for( int i = 0; i < size ;i++ ) {
    if ((data[i] == 0xff && data[i+1] == 0xd8 && data[i+2] == 0xff && 
        data[i+3] == 0xe0) ...

}

When you get to the last 2 iteration (i == size - 2), data[i+2] and data[i+3] will be out of bounds, so you should check that before accessing data[i+2] and data[i+3]. And the same is true for the last iteration, data[i+1] will be also out of bounds. Like I said, I don't know how jpeg works so I don't know the meaning of all these values (like 0xff or 0xd8). But I think that after the save_jpeg(data[start], length, name); you probably need to do a return.