CS50 Recover Segmentation Fault issue

Question

The goal of this program is recover JPGs from a file.

I've been working on this problem for about four or five days in the CS50 online class and I just cannot figure it out. I continue to get a segmentation fault and I have no idea why.

I've tried debug50 and find that I get the fault when the program tries to write to a new file. Why it does this I cannot figure out.

I've been bashing my head up against a wall on this one and I've completely erased and rewritten it multiple times. Any help would be greatly appreciated.

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

int main(int argc, char *argv[])
{
    if (argc != 2)
    {
        fprintf(stderr, "Usage ./recover file.type\n");
        return 1;
    }

    char *infile = argv[1];
    FILE *inptr = fopen(infile, "rb");


    if (inptr == NULL)
    {
        fprintf(stderr, "Could not open file designated to be recovered\n");
        fclose(inptr);
        return 2;
    }

    int counter = 0;
    FILE *img;

    uint8_t buffer[512];

    while (fread(buffer, sizeof(*buffer), 512, inptr))
    {
        if (buffer[0] == 0xff &&
            buffer[1] == 0xd8 &&
            buffer[2] == 0xff &&
            (buffer[3] & 0xf0) == 0xe0)
        {
            if (counter > 0)
            {
                fclose(img);
            }

            char filename[8];
            sprintf(filename, "%03i.jpg", counter);

            img = fopen(filename, "w");

            counter++;
        }

        if (counter !=0)
        {
            fwrite(buffer, sizeof(*buffer), 512, img);
        }
    }
    fclose(img);
    fclose(inptr);
    return 0;
}

Answer 1

char filename[7];
sprintf(filename, "%03i.jpg", counter);

A seven character string takes up 8 chars due to the NUL-terminator \0. Make the array larger so you don't write past the end of it.

if(img == NULL)
{
    fprintf(stderr, "Could not create image file\n");
    fclose(img);
    return 3;
}

If the img didn't open you don't need to close it. On the other hand, if it did open then you do need to close it. Move the fclose() call to the end of the loop.

Answer 2

In addition to the answer above,

Look at the syntax of the fwrite function:

https://en.cppreference.com/w/c/io/fwrite

size_t fwrite( const void *buffer, size_t size, size_t count,
               FILE *stream );

According to the documentation, the size parameter is the size of each value in the buffer.

In your code, you have:

fwrite(buffer, 512, 1, img);

The problem is obvious.

It looks like you do the same for fread. The syntax of the function is:

https://en.cppreference.com/w/c/io/fread

size_t fread( void          *buffer, size_t size, size_t count,
              FILE          *stream );

In your code you do:

fread(buffer, 512, 1, inptr)

But it should be

fread(buffer, sizeof *buffer, 512, inptr)

---

As an aside, when dealing with such files, I recommend opening them in binary mode so as to not tamper with the data being read.

FILE *inptr = fopen(infile, "rb");

---

Finally you should make use of the return value of fread which tells you the number of bytes that was actually read. You can then make use of this value in fwrite to make sure you write the correct number of bytes.

Answer 3

Okay, so I figured out that it was the NOT operator that I had put in the last if statement when what I should have put was:

    if (counter != 0)
    {
        fwrite(buffer, sizeof(buffer), 1, img);
    }

I am however still confused as to why the program returned a segmentation fault.

My understanding is that when the program reached this particular if statement it would not execute because the counter would be evaluated as false.

Wouldn't the program then just end after reading the whole input file returning 0?

Where does the segmentation fault come from?