What causes the segmentation fault in the following piece of code?-CodePudding

I have another file (card.raw) that I have to check for deleted images. I read a buffer into memory then check if it is the start of an image and if it is I write the first buffer, if it is not the start of a new image it keeps on writing till the next image starts. The images are back to back in the card.raw file. I have placed a few printf functions in the code and I have isolated the segfault to the last fwrite function but I have no idea what is causing it. I have tried Valgrind but I am not sure what the output means or how to fix it.

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

int main(int argc, char *argv[])
{
    FILE *raw = fopen(argv[1], "r");
    int SIZE = sizeof(raw);
    int buffer[512];
    int JPEG_num = 0;
    FILE *img[50];
    char filename[4];

    for(int j = 0; j < SIZE; j  )
    {
        for(int i = 0; i < 512; i  )
        {
            fread(&buffer[i], 1, 1, raw);
        }
        if(buffer[0] == 0xff)
        {
            if(buffer[1] == 0xd8)
            {
                if(buffer[2] == 0xff)
                {
                    if(buffer[3] >= 0xe0 && buffer[3] <= 0xef)
                    {
                        if(JPEG_num == 0)
                        {
                            sprintf(filename, "i.jpg", 0);
                            img[0] = fopen(filename, "w");
                            fwrite(&buffer, 1, 512, img[0]);
                            JPEG_num  ;
                        }
                        else
                        {
                            fclose(img[0]);
                            sprintf(filename, "i.jpg", JPEG_num);
                            img[JPEG_num] = fopen(filename, "w");
                            fwrite(&buffer, 1, 512, img[JPEG_num]);
                            JPEG_num  ;
                        }
                    }
                }
            }
        }
        else
        {
            if(JPEG_num != 0)
            {
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                JPEG_num  ;
            }
        }
    }
    fclose(img[JPEG_num]);
}

EDIT

I changed the filename size and the if conditions and the SIZE integer to the size of a pointer(I'm not sure if I did this correctly like suggested)

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

int main(int argc, char *argv[])
{
    FILE *raw = fopen(argv[1], "r");
    int SIZE = sizeof(*raw);
    int buffer[512];
    int JPEG_num = 0;
    FILE *img[50];
    char filename[9];

    for(int j = 0; j < SIZE; j  )
    {
        for(int i = 0; i < 512; i  )
        {
            fread(&buffer[i], 1, 1, raw);
        }
        if(buffer[0] == 0xff && buffer[1] == 0xd8 && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        {
            if(JPEG_num == 0)
            {
                sprintf(filename, "i.jpg", 0);
                img[0] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[0]);
                JPEG_num  ;
            }
            else
            {
                fclose(img[0]);
                sprintf(filename, "i.jpg", JPEG_num);
                img[JPEG_num] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                JPEG_num  ;
            }
        }
        else
        {
            if(JPEG_num != 0)
            {
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                JPEG_num  ;
            }
        }
    }
    fclose(img[JPEG_num]);
}

EDIT

I removed the int SIZE = sizeof(*raw); and just changed the for(int j = 0; j < SIZE; j ) loop to a while (fread(buffer, 1, 512, raw) == 512) loop

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

int main(int argc, char *argv[])
{
    FILE *raw = fopen(argv[1], "r");
    int buffer[512];
    int JPEG_num = 0;
    FILE *img[50];
    char filename[260];

    while( fread(buffer, 1, 512, raw) == 512 )
    {
        for(int i = 0; i < 512; i  )
        {
            fread(&buffer[i], 1, 1, raw);
        }
        if(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        {
            printf("A\n");
            if(JPEG_num == 0)
            {
                sprintf(filename, "i.jpg", 0);
                img[0] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[0]);
            }
            else
            {
                fclose(img[JPEG_num - 1]);
                sprintf(filename, "i.jpg", JPEG_num);
                img[JPEG_num] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                JPEG_num  ;
            }
        }
        else
        {
            if(JPEG_num != 0)
            {
                fwrite(&buffer, 1, 512, img[JPEG_num]);
            }
        }
    }
    fclose(img[JPEG_num]);
}

EDIT

I placed a few printf functions to see where the problem lies. It just prints out 1 and 2. It never enters the if condition.

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

int main(int argc, char *argv[])
{
    FILE *raw = fopen(argv[1], "r");
    int buffer[512];
    int JPEG_num = 0;
    FILE *img[50];
    char filename[260];

    while( fread(buffer, 1, 512, raw) == 512 )
    {
        printf("1\n");
        for(int i = 0; i < 512; i  )
        {
            fread(&buffer[i], 1, 1, raw);
        }
        printf("2\n");
        if(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        {
            printf("A\n");
            if(JPEG_num == 0)
            {
                printf("B\n");
                sprintf(filename, "i.jpg", 0);
                printf("C\n");
                img[0] = fopen(filename, "w");
                printf("D\n");
                fwrite(&buffer, 1, 512, img[0]);
                printf("E\n");
            }
            else
            {
                printf("!\n");
                fclose(img[JPEG_num - 1]);
                printf("@\n");
                sprintf(filename, "i.jpg", JPEG_num);
                printf("#\n");
                img[JPEG_num] = fopen(filename, "w");
                printf("^\n");
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                printf("&\n");
                JPEG_num  ;
            }
        }
        else
        {
            if(JPEG_num != 0)
            {
                printf("3\n");
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                printf("4\n");
            }
        }
    }
    fclose(img[JPEG_num]);
}

EDIT

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

int main(int argc, char *argv[])
{
    FILE *raw = fopen(argv[1], "r");
    int buffer[512];
    int JPEG_num = 0;
    FILE *img[50];
    char filename[260];

    while (fread(buffer, 1, 512, raw) == 512)
    {
        if(buffer[0] == 0xff && buffer[1] == 0xd8 && buffer[2] == 0xff && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        {
            if(JPEG_num == 0)
            {
                sprintf(filename, "i.jpg", 0);
                img[0] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[0]);
            }
            else
            {
                fclose(img[JPEG_num - 1]);
                sprintf(filename, "i.jpg", JPEG_num);
                img[JPEG_num] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                JPEG_num  ;
            }
        }
        else
        {
            if(JPEG_num != 0)
            {
                fwrite(&buffer, 1, 512, img[JPEG_num]);
            }
        }
    }
    fclose(img[JPEG_num]);
}

CodePudding user response：

"What causes the segmentation fault..."

There are several places that have potential for segmentation fault. One that stands out is this:

char filename[4];
...
sprintf(filename, "i.jpg", 0);

In this example, filename has enough space to contain 3 characters nul terminator. It needs to be declared with at least 8 to contain the result of "i.jpg", 0. (which if given enough space will populate filename with 000.jpg.)

If you are not working on a small embedded microprocessor, there is no reason not to create a path variable with more than enough space. Eg:

char filename[PATH_MAX];//if PATH_MAX is not defined, use 260

Note, that writing to areas of memory that your process does not own invokes undefined behavior, which can come in the form of segmentation fault, or worse, seem to work without a problem. For example, if your code happens to get by the point of writing a deformed value into the filename variable, and that variable is then used later to open a file:

img[0] = fopen(filename, "w");

it is unknown what the result will be. because your code does not check the results of this call, more potential for problems exists.

Edit to address size of file...

int SIZE = sizeof(*raw);

Does not provide the size of the file. It will return the sizeof a pointer, i.e. either 4 or 8 bytes depending on whether the application is built as 32 or 64 bit. Consider using something like this approach to get actual value for file size, resulting in a call such as:

unsigned long SIZE = fsize(argv[1]);

CodePudding user response：

In addition to what the other answers tell, the handling of file pointers is also broken:

        if(buffer[0] == 0xff && buffer[1] == 0xd8 && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        {  // We found a new header, lets create a new file...
            if(JPEG_num == 0)
            {
                sprintf(filename, "i.jpg", 0);
                img[0] = fopen(filename, "w");   // Open img[0]
                fwrite(&buffer, 1, 512, img[0]); // Write to img[0]
                JPEG_num  ;                      // JPEG_num is 1 ahead of the used index in `img` array!
            }
            else
            {
                fclose(img[0]);   // This will close the same FILE* again and again...
                sprintf(filename, "i.jpg", JPEG_num);
                img[JPEG_num] = fopen(filename, "w");  
                fwrite(&buffer, 1, 512, img[JPEG_num]);
                JPEG_num  ;
            }
        }
        else
        {  // No new header, just write
            if(JPEG_num != 0)  
            {  // Only write after we found first header
                fwrite(&buffer, 1, 512, img[JPEG_num]);  // OUCH! Remember: JPEG_num is 1 ahead of the index in `img` array.
                JPEG_num  ; // OUCH: We use same file but now JPEG_num is 2 or more ahead of index in `img` array.
            }
        }
    }
    fclose(img[JPEG_num]);

As a result, you are walking through your array way too fast.

Either use JPEG_num-1 and only increment after creating a new file,

Just remove the whole array and just use a single FILE *outfile; instead.

An improved version would be (Error checks to be added by OP):

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

int main(int argc, char *argv[])
{
    FILE *raw = fopen(argv[1], "rb");
    int buffer[512];
    FILE *outfile = NULL;
    char filename[9];
    int JPEGnum = 0;

    while (fread(buffer, 1, 512, raw) == 512)
    {
        if (buffer[0] == 0xff 
         && buffer[1] == 0xd8 
         && buffer[2] == 0xff
         && (buffer[3] >= 0xe0 && buffer[3] <= 0xef))
        { // We found a new header, let's create a new file...
            if (outfile != NULL)
            {
                fclose(outfile);
            }
            sprintf(filename, "i.jpg", JPEG_num);
            outfile = fopen(filename, "wb");  
            fwrite(buffer, 1, 512, outfile);
            JPEG_num  ;
        }
        else
        { // No new header, just write
            if (outfile != NULL)  
            {  // Only write after we found first header
                fwrite(buffer, 1, 512, outfile); 
            }
        }
    }
    if (outfile != NULL) // Check if we found at least one JPEG header
        fclose(outfile);
}

Here I also fixed the wrong loop over the size of a FILE data type instead of the file. Also the files are opened in binary mode.

CodePudding user response：

as ryker stated, there are several points of possible failures here.

another is int SIZE = sizeof(raw); sets SIZE to be the size of a pointer (4/8 bytes).