Pset4 Recover Creating Segmentation Fault

Question

Trying to work through Pset4 Recover. Currently getting a segmentation fault and not opening any new jpegs. My logic thinking here was to:

• Make sure user submits just 1 command line argument
• Open Card.raw
• Free up a 512-byte chunk in memory
• Read bytes from card.raw in 512 byte chunks
• If not the start of a Jpeg and haven't yet opened the first Jpeg, move on to the next 512 byte chunk
• If the start of a new Jpeg, write the 512 bytes into the new file
• If have opened a jpeg previously and not at the start of a new jpeg, append bytes to previously file
• Close card.raw and free up buffer memory

Where am I going wrong?

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

int main(int argc, char *argv[])
{
    // Check to make sure user submits just 1 command-line argument
    if (argc != 2)
    {
        return 1;
    }

    // Define file pointer and open file
    FILE *fptr = fopen(argv[1], "r");
    if (!fptr)
    {
        return 1;
    }

    // Pointer to 512 bytes in memory
    int *x = malloc(512);

    // Declare ints for first bytes in jpeg
    int a = 0;
    int b = 1;
    int c = 2;
    int d = 3;

    // Counter for number of jpegs
    int ctr = 0;

    while (!feof(fptr))
    {
        // Open space in memory to read bytes to
        unsigned char bytes[512];

        // Read 512 bytes from card.raw file
        fread(x, 1, 512, fptr);

        string filename = NULL;
        
        // Process for if haven't found the start of the first JPEG yet
        if (bytes[a] != 0xff && bytes[b] != 0xd8 && bytes[c] != 0xff && (bytes[d] & 0xf0) != 0xe0 & ctr == 0)
        {
            // Add to variables to move onto next 512 byte block after this
            a = a + 512;
            b = b + 512;
            c = c + 512;
            d = d + 512;  
        }

        // Process for if the first 4 bytes are the start of a new JPEG
        if (bytes[a] == 0xff && bytes[b] == 0xd8 && bytes[c] == 0xff && (bytes[d] & 0xf0) == 0xe0)
        {
            // Create new filename for jpeg file
            sprintf(filename, "%03i.jpg", ctr);

            // Open new file to hold jpeg
            FILE *img = fopen(filename, "w");

            // Write data from buffer to file
            fwrite(x, 1, 512, img);
            {
                if (*x < 255)
                {
                    return 1;
                }
            }

            // Add to counter
            ctr = ctr + 1;

            // Add to variables to move onto next 512 byte block after this
            a = a + 512;
            b = b + 512;
            c = c + 512;
            d = d + 512;

            // Close new JPEG file
            fclose(img);
        }

        // Process for if not the start of a new JPEG
        if (bytes[a] != 0xff && bytes[b] != 0xd8 && bytes[c] != 0xff && (bytes[d] & 0xf0) != 0xe0 & ctr >= 1)
        {
            // Open file to hold jpeg
            FILE *img = fopen(filename, "a");

            // Write data from buffer to file
            fwrite(x, 1, 512, img);
            {
                if (*x < 255)
                {
                    return 1;
                }
            }

            // Close JPEG file
            fclose(img);

            // Add to variables to move onto next 512 byte block after this
            a = a + 512;
            b = b + 512;
            c = c + 512;
            d = d + 512;
        }

    }

    // Close file
    fclose(fptr);
}

Answer 1

Kudos for making a good attempt, but you made a serious wrong turn. This logic is seriously flawed. The code effectively has two 512 byte buffers. It reads from the input file into the x buffer and then checks the bytes buffer for a signature. This is like looking in your front yard for a hammer that you lost in the basement, because the light is better. You're not going to find anything!

Putting that aside, whichever buffer is used, the code adds 512 to indexes a,b,c and d on each pass. After the first pass, the code is going to be using indexes that are out of range for whichever buffer is used. You can't access x+512 or bytes[1024], etc.

A major rewrite is needed here. Get rid of x as a buffer and use a single buffer of 512 bytes, like bytes[512]. There's no need to increment by 512. Each pass through the loop overwrites old data and writes a new 512 bytes into the buffer. Just check the first 4 bytes for a signature.

There may be other issues, but these are so significant that until you rewrite the program, there's no purpose in going any further. If you have problems after the rewrite, please post a new question.

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)