Segmentation fault in CS50 (2020) recovery program

Question

I am trying to write a program that will recover deleted images from a file and write each of those images to their own seperate files. I've been stuck on this problem for a few days, and have tried my best to solve it on my own, but I now realize I need some guidance. My code always compiles well, but everytime I run my program I suffer a segmentation fault. Using valgrind shows me that I don't have any memory leaks.

I think I have pinpointed the issue, though I'm not sure how to resolve it. When I run my program through the debugger, it always stops at the code inside my last 'else' condition (where the comment says "If already found JPEG") , and gives me an error message about the segmentation fault.

I have tried opening and initializing my file pointer jpegn atop this line of code, to prevent jpegn from being NULL when this condition is run, but that did not work to fix the fault.

I am very new to programming (and this site) so any advice or suggestions would be helpful.

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

typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    if(argc!=2) // Checks if the user typed in exactly 1 command-line argument
    {
        printf("Usage: ./recover image\n");
        return 1;
    }
    
    if(fopen(argv[1],"r") == NULL) // Checks if the image can be opened for reading 
    {
        printf("This image cannot be opened for reading\n");
        return 1;
    }
    
    FILE *forensic_image = fopen(argv[1],"r");  // Opens the image inputted and stores it in a new file
    
    BYTE *buffer = malloc(512 * sizeof(BYTE)); // Dynamically creates an array capable of holding 512 bytes of data
    
    if(malloc(512*sizeof(BYTE)) == NULL) // Checks if there is enough memory in the system
    {
        printf("System error\n");
        return 1;
    }
    
    // Creates a counting variable, a string and two file pointers
    
    int JPEG_num=0;
    char *filename = NULL;
   
    FILE *jpeg0 = NULL;
    FILE *jpegn = NULL;
    
    while(!feof(forensic_image))    // Repeat until end of image
    {
        fread(buffer, sizeof(BYTE), 512, forensic_image); // Read 512 bytes of data from the image into a buffer
        
        // Check for the start of a new JPEG file
        
        if(buffer[0] == 0xff & buffer[1] == 0xd8 & buffer[2] == 0xff & (buffer[3] & 0xf0) == 0xe0)
        {
            // If first JPEG
            
            if(JPEG_num == 0)
            {
                sprintf(filename, "%03i.jpg", JPEG_num);
                jpeg0 = fopen(filename, "w");
                fwrite(buffer, sizeof(BYTE), 512, jpeg0);
            }
            else    // If not first JPEG
            {
                fclose(jpeg0);
                JPEG_num++;
                
                sprintf(filename, "%03i.jpg", JPEG_num);
                jpegn = fopen(filename, "w");
                fwrite(buffer, sizeof(BYTE), 512, jpegn);
             }
            
        }
        else    // If already found JPEG
        {
            fwrite(buffer, sizeof(BYTE), 512, jpegn);
        }
        
        
    }
    
    // Close remaining files and free dynamically allocated memory

    fclose(jpegn);
    
    free(buffer);
    
}

Answer 1

There are quite many issues on your code. I am surprised if valgrind didn't identify these out.

First is this:

    if(fopen(argv[1],"r") == NULL) // Checks if the image can be opened for reading 
    {
        printf("This image cannot be opened for reading\n");
        return 1;
    }
    
    FILE *forensic_image = fopen(argv[1],"r");  // Opens the image inputted and stores it in a new file

This is not fatal, but you opened the same file twice and discard the first file pointer. But the one with similar pattern below is definitely a memory leak:

    BYTE *buffer = malloc(512 * sizeof(BYTE)); // Dynamically creates an array capable of holding 512 bytes of data
    
    if(malloc(512*sizeof(BYTE)) == NULL) // Checks if there is enough memory in the system
    {
        printf("System error\n");
        return 1;
    }

Here you allocated 512-bytes twice and keep only the first allocation in the pointer buffer, while the second allocation is lost.

And then this:

    char *filename = NULL;

    // ...   
    
    sprintf(filename, "%03i.jpg", JPEG_num);

you are writing a string to an unallocated memory!

and also the lines:

        else    // If already found JPEG
        {
            fwrite(buffer, sizeof(BYTE), 512, jpegn);
        }

How can you guarantee jpegn is a valid file pointer? Probably never because I see in your code that JPEG_num will always be 0. The part of else marked by // If not first JPEG is dead code.

Answer 2

when compiling, always enable the warnings, then fix those warnings.

gcc -ggdb3 -Wall -Wextra -Wconversion -pedantic -std=gnu11 -c "untitled1.c" -o "untitled1.o" 

results in several warnings like:

untitled1.c:46:91: warning: suggest parentheses around comparison in operand of ‘&’ [-Wparentheses]

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

Note: a single & is a bit wise AND. You really want a logical AND && for all but the last one in this statement

regarding;

FILE *forensic_image = fopen(argv[1],"r"); 

Always check (!=NULL) the returned value to assure the operation was successful. If not successful (==NULL) then call

perror( "fopen failed" ); 

to output to stderr both your error message and the text reason the system thinks the error occurred.

regarding:

while(!feof(forensic_image)) 

please read: why while( !feof() is always wrong

regarding:

FILE *forensic_image = fopen(argv[1],"r"); 

This is already done in the prior code block. There is absolutely no reason to do this again AND it will create problems in the code. Suggest: replacing:

if(fopen(argv[1],"r") == NULL)      
{         
    printf("This image cannot be opened for reading\n");
    return 1;     
} 

with:

if( (forensic_image = fopen(argv[1],"r") ) == NULL)      
{         
    perror( "fopen for input file failed" );         
    exit( EXIT_FAILURE );     
}

regarding:

BYTE *buffer = malloc( 512 * sizeof(BYTE) );

and later:

free( buffer );

This is a waste of code and resources. The project only needs one such instance. Suggest:

#define RECORD_LEN 512 

and

unsigned char buffer[ RECORD_LEN ]; 

regarding;

fread(buffer, sizeof(BYTE), 512, forensic_image); 

The function: fread() returns a size_t. You should be assigning the returned value to a size_t variable and checking that value to assure the operation was successful. Infact, that statement should be in the while() condition

regarding;

sprintf(filename, "%03i.jpg", JPEG_num); 

This results in undefined behavior and can result in a seg fault event because the pointer filename is initialized to NULL. Suggest:

char filename[20]; 

to avoid that problem

regarding:

else    // If not first JPEG             
{                 
    fclose(jpeg0); 

if your (for instance) working with the 3rd file, then jpeg0 is already closed, resulting in a run time error. Suggest removing the statement:

FILE *jpeg0;

and always using jpegn

regarding;

else    // If already found JPEG         
{             
    fwrite(buffer, sizeof(BYTE), 512, jpegn);         
} 

on the first output file, jpegn is not set, so this results in a crash. Again, ONLY use jpegn for all output file operations.

regarding:

fwrite(buffer, sizeof(BYTE), 512, jpegn); 

this returns the number of (second parameter) amounts actually written, so this should be:

if( fwrite(buffer, sizeof(BYTE), 512, jpegn) != 512 ) { // handle error } 

the posted code contains some 'magic' numbers, like 512. 'magic' numbers are numbers with no basis. 'magic' numbers make the code much more difficult to understand, debug, etc. Suggest using an enum statement or #define statement to give those 'magic' numbers meaningful names, then use those meaningful names throughout the code.