CS50 Recover (Pset4): Segmentation fault

Question

Basically title. I'm getting a segmentation fault instead of 50 jpeg images when I run my program.

I've tried playing around with different conditions.

Code:

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

const int BLOCK_SIZE = 512;
FILE *img = NULL;

int main(int argc, char *argv[])
{
    if (argc != 2) // Ensure correct usage of program
    {
        printf("Usage: ./recover IMAGE");
        return 1;
    }

    FILE *card = fopen(argv[1], "r");
    if (card == NULL) // Check if file was successfully opened
    {
        printf("Error! File cannot be opened");
        return 1;
    }

    typedef uint8_t block; // type for buffer
    block store[BLOCK_SIZE]; // Buffer for storing data from card.raw
    int count = 0; // Counter for file name
    char filename[8]; // Array to store filename

    while (fread(store, 1, BLOCK_SIZE, card) == BLOCK_SIZE)
    {

        if (store[0] == 0xff && store[1] == 0xd8 && store[2] == 0xff && ((store[3] & 0xf0) == 0xe0)) // If start of a new jpeg
        {
            if (count == 0)
            {
                count++;
                sprintf(filename, "%03i.jpg", count); // Write filename with sequential number
                img = fopen(filename, "w"); // Open img file to write to
                fwrite(store, 1, BLOCK_SIZE, img);
            }
            else if (count > 0)
            {
                fclose(img);
                count++;
                sprintf(filename, "%03i.jpg", count);
                img = fopen(filename, "w");
                fwrite(store, 1, BLOCK_SIZE, img);
            }
        }
        else
        {
            fwrite(store, 1, BLOCK_SIZE, img);
        }
    }
}

Edit: Sorry, messed up formatting the first time around. Code should be in now. Also, I'm now getting output that I can open, but the images all consist of a gray and white checkerboard pattern.

Welcome, but with no information, we can't be much help either. Please post a [Minimal Reproducible Example](https://stackoverflow.com/help/minimal-reproducible-example) as text, the shortest *complete* code that shows what you have tried. May I suggest you take the [Tour](https://stackoverflow.com/tour) and read [How do I ask a good question?](https://stackoverflow.com/help/how-to-ask) — Weather Vane, Nov 07 '22 at 20:23
Pease edit your _question_ and post your _entire_ code in a _single_ code block here. It should compile cleanly and be downloadable and runnable — Craig Estey, Nov 07 '22 at 20:52
Give that another shot. As is, there is no segmentation fault, but the only idea that I have for possibly fixing it produces a segmentation fault. My idea was to add another "fwrite(store, 1, BLOCK_SIZE, img);" inside the final else statement, to make the program continue writing to the image file if there is no new jpeg file. — PianoOwl, Nov 07 '22 at 21:30
In your previous comment, you stated that you tried adding `fwrite(store, 1, BLOCK_SIZE, img);` to the final else statement. If this is what causes the segmentation fault, then please add this information to the question, as this is important information. All important information should be in the question itself. It should not be necessary for someone wanting to answer the question to additionally read the entire comments section. — Andreas Wenzel, Nov 07 '22 at 21:50
I suggest that you post inside the question the code which is causing the segmentation fault, because you are correct that you should continue writing to `img` if the program doesn't detect a new JPEG. — Andreas Wenzel, Nov 07 '22 at 21:56
I suspect I know what is causing the segmentation fault, but I need to see your exact code to confirm my suspicion. Also, when I write an answer, I must answer the question as asked, but you are not asking about your code which causes the segmentation fault, because you are not showing that code in your question. Therefore, if you want me to tell you the reason for the segmentation fault, please [edit] your question to show the code which causes the segmentation fault. — Andreas Wenzel, Nov 07 '22 at 22:31
Ok, done. Current code produces the segmentation fault, and I updated the question. — PianoOwl, Nov 08 '22 at 03:32
When learning to program, learning to debug is an important and critical part of the process. For a helpful link, see [How to debug small programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/), consider a chat with the duck, and see [What is a debugger and how can it help me diagnose problems?](https://stackoverflow.com/q/25385173/3422102) — David C. Rankin, Nov 08 '22 at 04:48

Andreas Wenzel · Accepted Answer · 2022-11-08T04:12:10.517

In the Recover task of Problem Set 4 of CS50, the first two 512 byte blocks of the raw data do not yet contain the start of a JPEG file. Therefore, when running your program line by line in a debugger, you should observe the following:

When the program reaches the line

if (store[0] == 0xff && store[1] == 0xd8 && store[2] == 0xff && ((store[3] & 0xf0) == 0xe0))

for the first time, this condition will evaluate to false, because you have not yet reached the start of a JPEG file. Therefore, the else block will be executed instead, which contains the following line:

fwrite(store, 1, BLOCK_SIZE, img);

At this point, the debugger will show you that the value of img is still NULL. This means that you are passing an invalid FILE * to the function fwrite, which invokes undefined behavior and is probably the cause of the segmentation fault.

The simplest way to fix this would be to change the line

fwrite(store, 1, BLOCK_SIZE, img);

to:

if ( img != NULL )
{
    fwrite(store, 1, BLOCK_SIZE, img);
}

That way, the program will not attempt to write anything if a file has not yet been opened.

See this video link on how to run a debugger from CS50.

It is also worth noting that you have a significant amount of unnecessary code duplication in the following lines:

if (count == 0)
{
    count++;
    sprintf(filename, "%03i.jpg", count); // Write filename with sequential number
    img = fopen(filename, "w"); // Open img file to write to
    fwrite(store, 1, BLOCK_SIZE, img);
}
else if (count > 0)
{
    fclose(img);
    count++;
    sprintf(filename, "%03i.jpg", count);
    img = fopen(filename, "w");
    fwrite(store, 1, BLOCK_SIZE, img);
}

These lines can be simply rewritten to:

if ( img != NULL )
{
    fclose( img );
}

count++;
sprintf(filename, "%03i.jpg", count);
img = fopen(filename, "w");
fwrite(store, 1, BLOCK_SIZE, img);

That way, you have no code duplication.

Well written and very helpful to the OP learning to program (and debug). — David C. Rankin, Nov 08 '22 at 04:45
@DavidC.Rankin: Yes, I believe that showing a student the value of using a debugger is more helpful in the long run than the rest of my answer. Especially after OP wrote `"and the debugger isn't much help for this problem"` in [the initial version of their question](https://stackoverflow.com/revisions/74352503/1), I simply had to show OP that they were wrong. :-) — Andreas Wenzel, Nov 08 '22 at 14:30
A student that never learns how to use a debugger themselves will forever be dependent on other people to debug their programs for them, and will therefore never become a good programmer. — Andreas Wenzel, Nov 08 '22 at 14:39

CS50 Recover (Pset4): Segmentation fault

1 Answers1