0

My code compiles and saves 50 JPG images with proper names, but when I try to open any one of them, all of them are blank, just grid. When I run check50, I get Segmentation fault error.

What I'm doing wrong?

Here is pseudo-code:

- try to read 512 bytes from memory card
  - failure: 
    - exit
  - success: 
    - check first four bytes
        - not JPG's first four
          - output file already opened?
            - no: 
              - go to start (read next 512 bytes)
            - yes:
              - write to that file
              - go to start
        - JPG's first four:
          - output file already opened?
            - no: 
              - open new 
              - write to file
              - go to start
            - yes: 
              - close already opened output file
              - open new file
              - write to file
              - go to start

Here is actual code:

// recovers 'deleted' JPEGs from memory card

#include <stdio.h>
#include <cs50.h>
#include <stdbool.h>
#include <ctype.h>

// generate new file name
char *fileName(int i)
{
    char *filename = malloc(8);
    sprintf(filename, "%03i.jpg", i);
    return filename;
}

// open file
FILE *openFile(char *filename)
{
    return fopen(filename, "wb");
}

int main(int argc, char *argv[])
{
    // ensure proper usage
    if (argc != 2)
    {
        fprintf(stderr, "Provide a path to forensic image.\n");
        return 1;
    }

    char *infile = argv[1];

    // open input file
    FILE *inPointer = fopen(infile, "r");
    if (inPointer == NULL)
    {
        fprintf(stderr, "Could not open %s.\n", infile);
        return 2;
    }

    unsigned char buffer[512];
    int i = -1;
    int numberOfBytes;
    FILE *outputFilePointer = NULL;
    char *filename = NULL;

    while ((numberOfBytes = fread(buffer, 1, 512, inPointer)) > 0)
    {
        // check first 4 bytes
        if (buffer[0] == 0xff &&
            buffer[1] == 0xd8 &&
            buffer[2] == 0xff &&
            (buffer[3] & 0xf0) == 0xe0)
        {
            // already opened?
            outputFilePointer = openFile(filename);
            if (outputFilePointer != NULL)
            {
                // yes:
                // close file
                fclose(outputFilePointer);
                free(filename);
                // open new file;
                i++;
                filename = fileName(i);
                outputFilePointer = openFile(filename);
            }
            else
            {
                // no:
                // open new file
                i++;
                filename = fileName(i);
                outputFilePointer = openFile(filename);
            }
            // write to outputFile
            fwrite(buffer, 1, numberOfBytes, outputFilePointer);
        }
        else
        {
            // alredy opened?
            outputFilePointer = openFile(filename);
            if (outputFilePointer != NULL)
            {
                // yes:
                // write to outputFile
                fwrite(buffer, 1, numberOfBytes, outputFilePointer);
            }
                // no:
                // next read
        }
    }
    if (outputFilePointer != NULL)
    {
        fclose(outputFilePointer);
    }
    if (inPointer != NULL)
    {
        fclose(inPointer);
    }
    free(filename);
    return 0;
}

2 Answers 2

3

Mars is right, the code is too complex. Too much complexity around opening, closing, and testing for open files. It's understandable that when code is executed repeatedly, you'd want to create a function to put the repeating code in one place. You created two functions, one to create the filename and one to open the file. Since the filename function is ONLY called by the function that opens the new files, both of these activities should have been combined into a single function. It's more efficient to do so and eliminates the added overhead of a second function.

But, neither function is really necessary. If this program is correctly and efficiently implemented, it is only necessary to open new files in one place in the main code. (However, extra points for implementing functions, assumably correctly. It's a good skill to learn. )

Having said all that, I found the root problem. While I won't point it out exactly, I'll give you a hint. The problem lies in MARS and my original observation - way too many file opens used to do tasks that are easily done with simple checks, and to do other things.

Here's another big hint: Why open a file that's already open? What's actually being opened? ;-)

While the pseudo code may be good (I'm assuming it is for valid reasons), it's the actual implementation in the code thats causing trouble. The bottom line is this: "KISS - Keep It Simple..." ;-)

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

3
  • Just commented out outputFilePointer = fopen(filename, "w"); and problem is solved, thank you! Can you explain me briefly what happens when I open already opened file? Where bytes from buffer go in that case? Nov 11, 2018 at 14:13
  • It depends on how they are opened. If the file is reopened with "w", the existing file is erased and a new file with the same name is opened. Did you notice that all of the existing files were 512 bytes long?
    – Cliff B
    Nov 11, 2018 at 16:59
  • No, I didn't :( Nov 11, 2018 at 17:23
3

I think you complicate your program unnecessarily with calls to functions that do not clarify the code, recover can be done in a simpler way, and personally I always look for the greatest possible simplicity. Particularly suspicious I find the function

FILE * openFile (char * filename).

This function theoretically returns an open file, specifically a pointer to said file, but the name of the pointer is not explicitly declared, but is assigned later in the program. Problem, this pointer is local to the function and disappears when we leave it, so we have a pointer to nothing, with which we will have a segfault, or that you are reading random data from no one knows where. From my humble point of view I would look for a new algorithm and greater simplicity. The algorithm that you can use, in broad strokes, is if we find an image we close a possible open image (we simply check if it is NULL, establishing this initial value for the image). If we do not find an image we simply write. All this should not be difficult because we only use condions and loops.

7
  • I just replaced every openFile() function call with simpler fopen(filename, "wb");, and problem stays the same. Any other suggestion? Thank you. Nov 10, 2018 at 21:14
  • As for algorithm, I can try to come up with the simpler solution, but I think current is not complex at all. Can you take a look at the pseudo code if you haven't already? Nov 10, 2018 at 21:18
  • "wb" may be a new problem, I would simply use "a"
    – MARS
    Nov 10, 2018 at 21:46
  • You wouldn't want to use "a". That sets the opened file to append mode. The big problem here is that if the file already exists, it will be appended to, either making a good file into garbage, or adding more garbage to garbage. Just open it with "w" to create new files to write into.
    – Cliff B
    Nov 11, 2018 at 4:10
  • MARS, I agree completely. Too complicated. There's another issue buried a little deeper though, but I think you found it too. ;-) Points given.
    – Cliff B
    Nov 11, 2018 at 4:47

You must log in to answer this question.

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