Getting a segmentation fault in C, dont know why?

Question

I've also used Valgrind but still can't find the error. It's a CS50 problem to recover images.

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

typedef uint8_t BYTE;

int main(int argc, char *argv[])
{
    if ( argc != 2)
    {
        printf("Usage : ./recover image\n");
        return 1;
    }
    //Condition to check whether the file opens or not.
    FILE *file = fopen(argv[1], "r");
    int c=0;
    FILE *img;
    char *fileName = malloc(sizeof(char)*10);
    if(fileName == NULL)
    return 1;
        //Below is the dynamic reading of file.
    do
    {
        int *arr = malloc(sizeof(BYTE)*512);
        if(arr == NULL)
        return 1;         
        fread(arr,sizeof(BYTE),512,file);
        if(arr [0] == 0xff && arr[1] == 0xd8 && arr[2] == 0xff && (arr[3] & 0xf0) == 0xe0)
        {
            if(c == 0)
            {
                img = fopen(" 000.jpg","a");// check for w or a
                fwrite(arr,sizeof(BYTE),512,img);
            }
            else 
            {
                fclose(img);
                sprintf(fileName,"%03i.jpg",c);
                img = fopen(fileName,"a");//check for w or a
                fwrite(arr,sizeof(BYTE),512,img);
            }
            c++;
        }
        else 
        {
            if(c!=0)
            {
                img = fopen(fileName,"a");
                fwrite(arr,sizeof(BYTE),512,img);
            }
        }
            free(arr);
    }while(getc(file) != EOF);
    if(img != NULL)
        fclose(file);
    fclose(img);
    free(fileName);
}

screenshot with error message

*" Everything is irrelevant "* - that is not how you write a good question — UnholySheep, Jul 15 '20 at 12:23
use valgrind to find memory leaks, use a debugger like gdb and read the backtrace to solve runtime errors — TheDrev, Jul 15 '20 at 12:26
OT: immediately after: `FILE *file = fopen(argv[1], "r");` should be the checking for errors, not some statements later. So immediately after this statement should be: `if( ! file ) { perror( "fopen for input file failed" ); exit( EXIT_FAILURE ); }` This outputs to `stderr`, your error message AND the text reason the system thinks the error occurred. — user3629249, Jul 15 '20 at 15:16
regarding: `int *arr = malloc(sizeof(BYTE)*512); if(arr == NULL) return 1; ` This fails to inform the user that a problem occurred and fails to inform the user the details about the problem. Suggest: `int *arr = malloc(sizeof(BYTE)*512); if(arr == NULL) { perror( "malloc failed" ); fclose( file ); exit( EXIT_FAILURE ); }` Note: the expression: `sizeof( char )` is defined in the C standard as 1. Multiplying anything by 1 has not effect and just clutters the code. Suggest removing the expression: `sizeof(BYTE)` — user3629249, Jul 15 '20 at 15:22
Does this answer your question? [Exiting out of file with feof function](https://stackoverflow.com/questions/62886327/exiting-out-of-file-with-feof-function) — user3629249, Jul 15 '20 at 15:28
Please do not link to images. Rather copy/paste the information as text into your question — user3629249, Jul 15 '20 at 15:30
regarding: `printf("Usage : ./recover image\n");` Error messages should be output to `stderr`, not `stdout`. Note that an executable can be any name. Suggest: `fprintf( "USAGE: %s imagefile\n", argv[0] );` — user3629249, Jul 15 '20 at 15:34
regarding: `int *arr = malloc(sizeof(BYTE)*512);` this just introduces complications into your code. suggest: `char arr[ 512 ];` Similar considerations exist for `char *fileName = malloc(sizeof(char)*10);` can be `char fileName[ 10 ];` — user3629249, Jul 15 '20 at 15:37
regarding: `fread(arr,sizeof(BYTE),512,file);` always check for errors when performing I/O operations. Suggest: `if( fread(arr,sizeof(BYTE),512,file) != 512) { // handle error, cleanup, and exit }` Similar considerations exist for all the calls to `fread()` and `fwrite()` — user3629249, Jul 15 '20 at 15:40
OT: regarding: `int *arr = malloc(sizeof(BYTE)*512);` and `free( arr );` Why do this with each record read from the input file? It is OK to use the same allocated memory over and over for the same purpose. Also, all those calls to `malloc()` and `free()` will (tend) to fragment the available memory. — user3629249, Jul 15 '20 at 15:44
OT: regarding: `sprintf(fileName,"%03i.jpg",c);` This only needs to be performed ONCE for each new file, not once for each 512 byte record in the file. — user3629249, Jul 15 '20 at 15:46
OT: regarding: `int *arr = malloc(sizeof(BYTE)*512);` why a `int` pointer, especially as the input file is a binary file and consists of (mostly unprintable) characters — user3629249, Jul 15 '20 at 15:49
it is (usually) a good practice to not introduce a bunch of variables that contain the same information as other variables. Suggest removing the variable: `c`, initialize `img` to NULL, and if not NULL then some prior file needs to be closed — user3629249, Jul 15 '20 at 15:52

MemAllox · Answer 1 · 2020-07-15T17:48:05.577

Your problem is that your code is bad. The thing you're missing is error checking. Most library functions (like fopen, malloc, etc.) indicate via their return value if they succeeded. For instance fopen can fail if the file you want to open does not exist.

See the fopen documentation.

Upon successful completion fopen(), fdopen() and freopen() return a FILE pointer. Otherwise, NULL is returned and errno is set to indicate the error.

For every library call in your code, consider what happens if the function fails. You need to account for that case. The least you should do is print an error message and exit the program properly.

E.g. for fopen:

*file = fopen(argv[1], "r");
if (file == NULL) {
    fprintf(stderr, "fopen() failed: %s\n", strerror(errno));
    return 1;
}
// ...

Edit: if you want to become a software developer, you should look into these things:

error handling in C
how to use the GDB debugger to find segmentation faults and other bugs
how to ask good questions on stackoverflow

score 0 · Answer 2 · answered Jul 15 '20 at 13:58

0

Have you checked this out in your code?

int *arr = malloc(sizeof(BYTE)*512);

I don't see where you're freeing it. Try this and let me know, I can help

answered Jul 15 '20 at 13:58

Nicolas F

505
6
17

Getting a segmentation fault in C, dont know why?

2 Answers2