1

I've managed to make a working copy of CS50 resize. However, in order to adhere to the thought of not repeating myself, have tried to create a function to create a new file.

The program at present runs fine, and I managed to make a function that simply made and opened a new file prior to adding in all the other boolean conditions (it simply created 50 empty files). However my current code if I attempt to use the function (where I have indicated //start new file) causes a segmentation fault.

Has anyone got any advice? I seem to struggle with knowing how to pass back information from functions to make the code more tidy.

Ella

/**
* recover.c
*
* Computer Science 50
* Problem Set 4
*
* Recovers JPEGs from a forensic image.
* ./recover card.raw
*/

// libraries
#include <cs50.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

// definitions
#define BUFSIZE 512
typedef uint8_t BYTE;

// functions
bool find_JPEG(BYTE[]);
FILE* newfile(FILE*, char[], int, BYTE[]);

/**
 * main file
 */

 int main(int argc, char* argv[])
 {
    // check correct usage of program
    if(argc != 2)
    {
    fprintf(stderr, "Error: must be in format ./recover file.raw");
    return 1;
}

// open file
char* input = argv[1];
FILE* infile = fopen(input, "r");


// check file is in right format
if (infile == NULL)
{
    fprintf(stderr, "Could not open %s.\n", input);
    return 2;
}

/** 
 * run program with infile to restore JPGs
 */

// declare variables
BYTE buffer[BUFSIZE];
int JPEG_counter = 0;
char filename [8];
FILE* img = NULL;

// enter while loop to run 
while(!feof(infile) && fread(&buffer, BUFSIZE, 1, infile) == true)
{
    //  If start of new image identified
    if(find_JPEG(buffer) == true)
        {
            // special case if first image
            if(JPEG_counter == 0)
            {
                // start new file
                sprintf(filename, "%03i.jpg", JPEG_counter);
                img = fopen(filename, "w");
                fwrite(buffer, 1, BUFSIZE, img);
                JPEG_counter++;
            }
            // if not first image
            else
            {
               // close current file
               fclose(img);

               // start new file
                sprintf(filename, "%03i.jpg", JPEG_counter);
                img = fopen(filename, "w");
                fwrite(buffer, 1, BUFSIZE, img);
                JPEG_counter++; 
            }


        }
    // if no new JPEG found    
    if(find_JPEG(buffer) == false)
    {
        // continue to next buffer if no open file
        if(JPEG_counter == 0)
        {
            continue;
        }
        else
        // carry on writing to file otherwise
        {
            fwrite(buffer, 1, BUFSIZE, img);   
        }
    }

}  

if(feof(infile))
{
    fclose(infile);
}

return 0;   
}


/** 
 * declare functions for program to use
 */

bool find_JPEG(BYTE buffer[])
{
// determine if new JPEG present?
if  (buffer[0] == 0xff && 
    buffer[1] == 0xd8 &&
    buffer[2]== 0xff &&
    (buffer[3] & 0xf0) == 0xe0)
    {
        return true;
    }

    else
    {
        return false;
    }
}

/**
 * create new file
 */

FILE* newfile(FILE* img, char filename[], int JPEG_counter, BYTE buffer[])
{
    sprintf(filename, "%03i.jpg", JPEG_counter);
    img = fopen(filename, "w");
    fwrite(buffer, 1, BUFSIZE, img);
    return img;
}
3

Assuming that i understand correctly and the function causing the problem is newfile, the problem is this:

Your argument img is a FILE pointer. At the beginning of your code you set it to NULL, which is perfect. This means that the value stored in your pointer is 0x0. I'm just guessing here, because to really know we should be able to see how you called your function. But i'm guessing you did something like:

newfile(img, filename, JPEG_counter, buffer);

If you did this, the problem is that when you do:

img = fopen(filename, "w");

This modifies the value stored in the pointer: it allocates some memory, does whatever fopen does, and it stores the address of this new block into img. But if you're not storing the return when you call the function, img will not be modified. If you actually store it, it will work:

img = newfile(img, filename, JPEG_counter, buffer);

This stores the new value in img and all is well.

From now on there's a very long attempt at explaining the problem. What you did there is exactly the same as the no-swap example from Prof. Malan's video. Now, you might say "but img is a pointer!". It is a pointer, but what the swap example does is it modifies the value stored at the variable pointed by the pointer, not the address stored at the pointer itself. Forget for a second that i'm calling a function that's not been declared yet :D

int main(void) {
    int a = 5; // the value stored in a is 5
    int b = 4; // the value stored in b is 4
    int* a_ptr = &a; // the value stored in a_ptr is the address of a, let's suppose it's 0x123
    int* b_ptr = *b; // the value stored in b_ptr is the address of b, let's suppose it's 0x234

swap(a_ptr, b_ptr); // what you pass here is two int pointers, one having a value stored 0x123, the other 0x234
}

void swap (int* a , int* b) {
     // when we get here, what we have is a copy of the pointers (not the real pointers). These copies are also int pointers and the values stored are 0x123 and 0x234. 
    int temp = *a;
    *a = *b; // what we modify here is NOT the pointer, but the value stored at the variable that lives at address 0x123 and give it the value stored at address 0x234
    *b = temp;       }

In your case, what you're doing is: (removing the non related code)

int main(void) {
    FILE* img = NULL; // declare a FILE*. The value stored in it is 0x0
    newfile(img, filename, JPEG_counter, buffer); // passing it to the function, so you pass 0x0 as first argument. 
    fwrite(buffer, 1, BUFSIZE, img); // because you didn't store the returned pointer, img is still holding a value of 0x0, here's the segfault
}
FILE* newfile(FILE* img, char filename[], int JPEG_counter, BYTE buffer[])
{
    // what we have now is a COPY of img (not the same pointer, but a copy). It's a FILE* with a value stored of 0x0
    img = fopen(filename, "w");
    // now our local copy has a different value stored, let's suppose 0x123. 
    return img; // this is important, if you store this, you will actually return a pointer with a value 0x123 stored in it. If you don't store it, this variable gets destroyed 
}

Let me know if it solves the problem and if it was somewhat clear :)

1
  • Thanks this really helped - guess I need to brush up on my understanding of pointers though. I think it might take a while! – Ella Jennings Jan 6 '17 at 10:18
0

Thanks Irene, this really helped, with a couple of modifications.

1: I had to declare

   File*img = NULL;

prior to saying that

   img = newfile(img, filename, JPEG_counter, buffer);

(guess I should have figured out you need to declare it before you can call it)

  1. Also it was then not working on the very first image, which is because there is already an image open in any case (I guess) just by declaring it, therefore in all cases I have to close the previous file before opening a new one. I think/hope it makes it more readable!

This is my code now:

     /**
     * recover.c
     *
     * Computer Science 50
     * Problem Set 4
     *
     * Recovers JPEGs from a forensic image.
     * ./recover card.raw
     */

    // libraries
    #include <cs50.h>
    #include <stdio.h>
    #include <stdlib.h>
    #include <stdint.h>

    // definitions
    #define BUFSIZE 512
    typedef uint8_t BYTE;

    // functions
    bool find_JPEG(BYTE[]);
    FILE* newfile(FILE*, char[], int, BYTE[]);

    /**
     * main file
     */

    int main(int argc, char* argv[])
    {
        // check correct usage of program
        if(argc != 2)
        {
            fprintf(stderr, "Error: must be in format ./recover file.raw");
            return 1;
        }

        // open file
        char* input = argv[1];
        FILE* infile = fopen(input, "r");


        // check file is in right format
        if (infile == NULL)
        {
            fprintf(stderr, "Could not open %s.\n", input);
            return 2;
        }

    /** 
     * run program with infile to restore JPGs
     */

        // declare variables
        BYTE buffer[BUFSIZE];
        int JPEG_counter = 0;
        char filename [8];
        FILE* img = NULL;
        img = newfile(img, filename, JPEG_counter, buffer);

        // enter while loop to run  (fread returns >0 if running)
        while(!feof(infile) && fread(&buffer, BUFSIZE, 1, infile) == true)
        {
            //  If start of new image identified
            if(find_JPEG(buffer) == true)
                {
                       // close current file
                       fclose(img);

                       // start new file
                        img = newfile(img, filename, JPEG_counter, buffer);
                        JPEG_counter++; 
                }

            // if not the start of a new image    
            if(find_JPEG(buffer) == false)
            {
                //if no image already started
                if(JPEG_counter == 0)
                {
                    continue;
                }

                // if already writing a file
                else
                {
                    fwrite(buffer, 1, BUFSIZE, img);   
                }
            }

        }  

        if(feof(infile))
        {
            fclose(img);
            fclose(infile);
        }

     return 0;   
    }


    /** 
     * declare functions for program to use
     */

    bool find_JPEG(BYTE buffer[])
    {
        // determine if new JPEG present?
        if  (buffer[0] == 0xff && 
            buffer[1] == 0xd8 &&
            buffer[2]== 0xff &&
            (buffer[3] & 0xf0) == 0xe0)
            {
                return true;
            }

            else
            {
                return false;
            }
    }

    /**
     * create new file
     */

    FILE* newfile(FILE* img, char filename[], int JPEG_counter, BYTE buffer[])
    {
        sprintf(filename, "%03i.jpg", JPEG_counter);
        img = fopen(filename, "w");
        fwrite(buffer, 1, BUFSIZE, img);
        return img;
    }
3
  • Won't that create an empty filename with no meaningful name in it though? I mean this: BYTE buffer[BUFSIZE]; int JPEG_counter = 0; char filename [8]; FILE* img = NULL; img = newfile(img, filename, JPEG_counter, buffer); – Irene Jan 7 '17 at 0:03
  • I think the way I explained it would make you think there's an empty file, but not looking at the code? The first file declared has a JPEG_counter of 0 and it's set to continue if jpeg_counter is 0. When I run this code as stated, I get files 000 to 049. Looking at the //declare variable declaration, it seems to have the identical code to what you've just written? – Ella Jennings Jan 24 '17 at 19:43
  • Oh! I've just finally understood what you meant. Fixed by removing any declaration of filename outside the function and declaring it in the first line of FILE* newfile(FILE* img, int JPEG_counter, BYTE buffer[]) – Ella Jennings Feb 8 '17 at 9:58

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .