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

Question

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, "%03i.jpg", 0);
                            img[0] = fopen(filename, "w");
                            fwrite(&buffer, 1, 512, img[0]);
                            JPEG_num++;
                        }
                        else
                        {
                            fclose(img[0]);
                            sprintf(filename, "%03i.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, "%03i.jpg", 0);
                img[0] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[0]);
                JPEG_num++;
            }
            else
            {
                fclose(img[0]);
                sprintf(filename, "%03i.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, "%03i.jpg", 0);
                img[0] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[0]);
            }
            else
            {
                fclose(img[JPEG_num - 1]);
                sprintf(filename, "%03i.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, "%03i.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, "%03i.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, "%03i.jpg", 0);
                img[0] = fopen(filename, "w");
                fwrite(&buffer, 1, 512, img[0]);
            }
            else
            {
                fclose(img[JPEG_num - 1]);
                sprintf(filename, "%03i.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]);
}

Prime suspect would be `JPEG_num` going out of range of the array, or the initialized portion of the array. Also, be sure to check the result of `fopen()` calls. — Fred Larson, Oct 05 '21 at 12:31
Have you compiled with all warning turned on? Or run in a debugger ? Either of these have the ability to immediately point to the problem. — ryyker, Oct 05 '21 at 12:34
I guess this code should solve that CS50 question where you need to find JPG files in a raw file. Your `if`-cascade is not correct. You need an `else` on every level of the header detection. Or you just use one single `if` — Gerhardh, Oct 05 '21 at 12:35
`filename` is not big enough. Every `sprintf()` call on it is going off the end of the buffer. It needs a size of at least 8. — Fred Larson, Oct 05 '21 at 12:35
`char filename[4];`. You're actually storing more than 4 bytes in it. And you should question the `int SIZE = sizeof(raw)`... — Zilog80, Oct 05 '21 at 12:35
With no options, latest Clang is providing a useful warning: `warning: 'sprintf' will always overflow; destination buffer has size 4, but format string expands to at least 8`. With warnings turned up, GCC provides a similarly useful message: `warning: '%03i' directive writing between 3 and 10 bytes into a region of size 4`. — Thomas Jager, Oct 05 '21 at 12:37
What do you mean I should question the int SIZE = sizeof(raw)? — Dylan, Oct 05 '21 at 12:39
If using `gcc` for example, just include `-Wall` to turn all warnings on. — ryyker, Oct 05 '21 at 12:39
I changed the filename size to 8 and I used a single 'if' condition to spot the beginning of a jpg file but it still gives a segfault — Dylan, Oct 05 '21 at 12:45
`for(int i = 0; i < 512; i++){ fread(&buffer[i], 1, 1, raw);}` would be better written with a single read `fread(buffer, 1, 512, raw);` — William Pursell, Oct 05 '21 at 12:47
@WilliamPursell , I Had it like that when I started but if I do it like that then I'm not sure how to check the start of the buffer for the start of an image. — Dylan, Oct 05 '21 at 13:01
@Dylan Changing the loop to a single read will not change anything. But you do need to check the value returned by `fread`. You need to do that in your for loop also, and it's a bit easier with a single read. — William Pursell, Oct 05 '21 at 13:05
Your loop on SIZE should probably be `while( fread(buffer, 1, 512, raw) == 512 )`. You cannot know how much data is in the file unless you either read it or you stat the file. But stat'ing the file will fail when the file is a pipe, so it's best to just read until you get no more data. — William Pursell, Oct 05 '21 at 13:14
Why do you use an array for files? How many do you keep open at any time? More than 1? And why do you only close `fclose(img[0]);` for every new header you find? — Gerhardh, Oct 05 '21 at 13:22
Can you explain what this line is doing? `for(int j = 0; j < SIZE; j++)` I don't think it can possibly be correct. The size of the `FILE`, which is an opaque data structure if I recall correctly, should not affect your code. — Tim Randall, Oct 05 '21 at 13:25
@TimRandall - It is supposed to iterate through the card.raw file — Dylan, Oct 05 '21 at 13:31
@Dylan the map is not the terrain. The `FILE` structure is just a way of managing access to a file. It's full of flags and control stuff. It doesn't contain every byte in the actual file. It probably doesn't contain any of them. To find the actual size of the file, see the comment from William Pursell above ("Your loop on SIZE...") — Tim Randall, Oct 05 '21 at 13:37
Now you read 512 bytes twice and discard the first buffer and usage of JPEG_num is messed up now. — Gerhardh, Oct 05 '21 at 14:07
@Gerhardh - Isn't that what is supposed to happen? To only use the buffers that are valid? — Dylan, Oct 05 '21 at 14:10
No, you should check every block for the header, not skip one and check one. You should only have one call of `fread` in your code. — Gerhardh, Oct 05 '21 at 14:20
Run it in a debugger. Check how many times you read, what value `JPEG_num` has in each step and compare with expected values. That's still broken. — Gerhardh, Oct 05 '21 at 14:31
It is not a good idea to ever change the code you originally posted with the exception of formatting for readability. The moment you _update_ your code, the problems that by now have been addressed by commenters, and by answers are all made obsolete, and other people coming into look become confused by the apparent incongruity between your _new_ post, and all of the comments/answers that addressed the _original_ post. Please do _not_ edit for the purpose of migrating the problems out of your code. If issues are addressed, and there are more questions, post _another_ question. — ryyker, Oct 05 '21 at 14:39
I’m voting to close this question because the issues of the original question have been addressed, but post continues to grow with additional questions. Evolving a question beyond its original scope is not in scope for this community. — ryyker, Oct 05 '21 at 14:44

ryyker · Answer 1 · 2021-10-05T13:45:04.740

1

"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, "%03i.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 "%03i.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]);

edited Oct 05 '21 at 13:45

answered Oct 05 '21 at 12:43

ryyker

22,849
3
43
87

I changed the filename size but it still gave me a segmentation fault. – Dylan Oct 05 '21 at 13:02
@Dylan - where? Did you run your code in debug mode and step through? – ryyker Oct 05 '21 at 13:09
I edited it to char filename[260]; and it still gives a segfault at the same place. – Dylan Oct 05 '21 at 13:10
It's in the ```fwrite(&buffer, 1, 512, img[JPEG_num]); ``` line. The last one – Dylan Oct 05 '21 at 13:11
I have noticed that possibility and I will fix this, I just wanted to get the code correct before checking for the correct input. I changed my code to have a larger filename but it still gives a segfault. What else could it be? – Dylan Oct 05 '21 at 13:22
@Dylan - without having the exact file to read into `argv[1]` I don't know. I just ran your code for a generic file input, and removing all the condition statements down to the `ifif(JPEG_num == 0)...else` clause, both sections ran without issue. The value you assign to `SIZE` is not right. I would check that. See my edit... – ryyker Oct 05 '21 at 13:33
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 – Dylan Oct 05 '21 at 13:50
1

@Dylan ...with what results? – ryyker Oct 05 '21 at 13:52
still none, the segmentation fault remains. Please see updated version of edited code. – Dylan Oct 05 '21 at 13:58
@Dylen - I am also going to repeat this comment under your post, It is not a good idea to ever change the code you originally posted with the exception of formatting for readability. The moment you _update_ your code, the problems that by now have been addressed by commenters, and by answers are all made obsolete, and other people coming into look become confused by the apparent incongruity between your _new_ post, and all of the comments/answers that addressed the _original_ post. Please do _not_ edit for the purpose of migrating the problems out of your code. – ryyker Oct 05 '21 at 14:38
I changed my code to your and @Gerhardh's recommendations but it still gives segmentation fault. please see latest edit to the question – Dylan Oct 05 '21 at 14:50
1

@Dylan - Have you taken steps to do any of the other suggestions that have been made? Have you stepped through with a debugger? Have you compiled with all warnings enabled. The benefit to you will be much greater taking these suggestions along with the input you have received in terms of answer/comments and troubleshoot the rest your self. If in the process of doing this you discover _another_ problem that you cannot answer, then certainly come back and post _another_ question regarding that new problem. – ryyker Oct 05 '21 at 15:02

Gerhardh · Answer 2 · 2021-10-05T14:03:23.883

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, "%03i.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, "%03i.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,

or

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, "%03i.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.

I changed it to JPEG_num - 1 and removed the last JPEG_num ++ — Dylan, Oct 05 '21 at 13:41
Then you need to provide another updated version. Or you could test my improved version. — Gerhardh, Oct 05 '21 at 13:49
Did you intend to suggest OP edit the post to change the problematic code? — ryyker, Oct 05 '21 at 13:51
@ryyker yes, in the same way the OP already **added** a first updated version. — Gerhardh, Oct 05 '21 at 13:52
I ran your code and it compiles fine and looks as if it runs fine but it doesn't create any jpg files. — Dylan, Oct 05 '21 at 13:53
As I don't have your test file, of course I did not test the code. There is always some time for every C developer where they need to run a debugger and follow all steps in the program. — Gerhardh, Oct 05 '21 at 13:54
Yes, I just noticed that had happened. Nevertheless, it is never a good suggestion to modify anything other than formatting in the original post. It results in several people who are coming into look at a problem that is altogether now different than the original, making all the comment and answer content irrelevant, and confusing. — ryyker, Oct 05 '21 at 13:57
@ryyker If the update are clearly marked as such, I don't see that as a problem. — Gerhardh, Oct 05 '21 at 13:58
@Gerhardh - Is there a way I could post the card.raw file with the question? — Dylan, Oct 05 '21 at 14:01
As I mentioned, run the code in a debugger and watch closely where program execution goes along and how variables change — Gerhardh, Oct 05 '21 at 14:34

score 0 · Answer 3 · answered Oct 05 '21 at 12:48

0

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).

answered Oct 05 '21 at 12:48

Tomer W

3,395
2
29
44

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

3 Answers3