0

I have spent a long time looking through my code for a segmentation fault, but have not been able to find it.

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

#define BLOCK 512

typedef uint8_t  BYTE;

int main(void)
{
//opens card.raw file
FILE* card = fopen("card.raw", "r");

//variables
int jpgcount = 0;
BYTE buff[BLOCK];
FILE* img;
char title[10];


//finds first instance of jpg
do
{
    BYTE buff[BLOCK];
    for (int i = 0; i < BLOCK; i++)
    {
        fread(&buff[i], sizeof(BYTE), 1, card);
    }
}
while(buff[0]==0xff && buff[1]==0xd8 && buff[2]==0xff && (buff[3]==0xe0 || buff[3]==0xe1));

//recovers jpgs
while (fread(&buff[0], BLOCK, 2, card) != 1)
{
    //checks for new jpg
    if (buff[0]==0xff && buff[1]==0xd8 && buff[2]==0xff && (buff[3]==0xe0 || buff[3]==0xe1))
    {
        if (img != NULL)
            fclose(img);
        sprintf(title, "%04d.jpg", jpgcount);
        img = fopen(title, "w");
        fwrite(&buff[0], BLOCK, 1, img);
    }
    else //writes remaining jpg
    {
        fwrite(&buff[0], BLOCK, 1, img);
    }

    // reads a block of info
    BYTE buff[BLOCK];
    for (int i = 0; i < BLOCK; i++)
    {
        fread(&buff[i], sizeof(BYTE), 1, card);
    }
}

//closes remaining files
fclose(card);
fclose(img);
}
  • Did you try running valgrind? What output did it give you? – Luke Van In Jul 22 '14 at 8:46
6

Issues with your program:

First, you're reserving 10 chars for the name of the JPG while the maximum length of the name of any JPG is just 7 (e.g., 0xy.jpg) + 1 for the null terminator (i.e., '\0').

Second, within your for loop

for (int i = 0; i < BLOCK; i++)
{
    fread(&buff[i], sizeof(BYTE), 1, card);
}

you're reading 512 bytes, but you're doing that byte-by-byte while you could do something as easy as this

fread(&buff, BLOCK, 1, card); // reading 1 element of size BLOCK

or this

fread(&buff[i], 1, BLOCK, card); // reading BLOCK elements of size 1 byte

This would probably make things easier as you'd access the memory less times.

Third, the first do..while loop is meant to find the first instance of a JPG. You're reading at least once and you're telling your program to keep reading as long as the continuation condition is true.

So after your first read, if you found a start of a JPG, you'll continue reading and that probably could make you lose some data! Besides, you're never checking whether the EOF is reached which could lead to some issues.

In the second while loop, you're reading 2 elements of size BLOCK for no obvious reasons for me too, but more importantly, you're reading this into a buffer of size BLOCK which is as less as half the memory you need. So you're writing on memory that you don't own and that's a reason that could make your program segfault.

Also, per the manual page forfread()

On  success,  fread()  and  fwrite() return the number of items read or
    written.

so probably you should check if you're expecting the correct the return value upon success.

You're also opening img with the mode w while it should be opened with the mode a. Per the manual page for fopen()

w      Truncate file to zero length or create text  file  for  writing.
       The stream is positioned at the beginning of the file.

a      Open  for  appending (writing at end of file).  The file is cre‐
       ated if it does not exist.  The stream is positioned at the  end
       of the file.

Besides, you've never opened img before (unless the if body is executed), so if your program to execute the else block first, you'll get into troubles and that's another reason that might be causing your program to segfault.

Lastly, you're declaring another array with the same name (i.e., buff) and you're reading data into it while you shouldn't do that because the second one shadows the original one and as soon as the closing brace of the while loop is reached, the second one goes out of scope and you lose all the data in it.

Besides, what you'll be really using from this moment on (i.e., after the closing brace of the loop is reached) will be the original buff (i.e., the one declared atop your program) until the duplicated one is declared and the process repeats as the loop body executes.

And again, you're never checking for the EOF and that could lead to some troubles too.


A correct approach

// find the start of the first JPG
do
    read a block
while this block is not a start of a jpg AND the EOF is not reached

while the EOF is not reached
{
    // always yes for the first iteration
    if the read block is a start of a JPG
    {
        close the jpg file if open
        update the name and open a new one
    }

    write the block
    read the next block
}

close the last JPG file // important
  • But I used a char array with size 16 and it worked. – toyotasupra Sep 10 '17 at 10:14

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .