Segmentation fault core dumped when reading file

Question

I got segmentation fault (core dumped) error in line 24 (fgets). I'm not very familliar with c, but I had to make a program for my classes. I have following code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>

int main(int argc, char* argv[]){
   FILE *fd1, *fd2;
   char *str1, *str2;
   char *salt, *hash, *key, *key1;
   char buf[13], word[200], pass[200];

   if(argc != 2){
       fprintf(stderr, "Usage: %s <file shadow>\n", argv[0]);
       exit(1);
   }

   str1 = (char*) malloc(100);
   str2 = (char*) malloc(100);

   fd1 = fopen(argv[1], "r");

   fprintf(stderr, "Please, wait...\n");

   while(fgets(str1, 100, fd1) != NULL){
       str2 = strstr(str1, "$1$");
       if(str2 != NULL){
           key = strtok(str2, ":");
           snprintf(pass, sizeof(pass), "%s", key);
           printf("pass=%s (%lu)\n", pass, strlen(pass));

           strtok(key, "$");
           salt = strtok(NULL, "$");
           hash = strtok(NULL, "\0");

           snprintf(buf, sizeof(buf), "$1$%s$", salt);

           fd2 = fopen("polish.txt", "r");

           while(fgets(word, 200, fd2) != NULL){
               (&word[strlen(word)])[-1] = '\0';

           key1 = crypt(word, buf);

           if(!strncmp(key1, pass, strlen(key1))){
               printf("OK!, The password is: %s\n\n", word);
               break;
           }


           }
       }

   }
   fclose(fd1);
   fclose(fd2);
   free(str1);
   free(str2);

   return 0;
 }

When I try to read a /etc/shadow file it throws me segmentation fault (tried with custom txt file too). Could anyone take a look at this?

Answer 1

There are couple of things you can improve in your code,

1. You don't need to cast malloc, it's not needed and may lead to untrackable bugs.

2. You don't need to use malloc for a local variable of fixed size.

3. You never check for the return value of any of the functions which return NULL on failure, in your case these:

   1. malloc()
   2. fopen()
   3. strok()

all these functions return NULL on failure.

4. You malloc and assing the pointer str2, but then you overwrite it at

    str2 = strstr(str1, "$1$");
   

there is no point in doing that, it means that you don't understand how pointers work. strstr() returns a pointer to the same string passed to it, just incremented to point to the begining of the substring you are looking for, or NULL if it wasn't found.

5. You have

    key = strtok(str2, ":");
    /* some other code */
    strtok(key, "$");
   

that's wrong because you are passing an incremented pointer to the same string, you must do it this way, read strtok()

    strtok(NULL, "$");

6. You fclose the fd2 I/O stream outside the while loop, but you fopen() it inside, you might have fopened it many more times than those you fclosed it.

You have two options, either you move the fclose() inside the while loop, or you move the fopen() outside the while loop, the second one is of course, better.

So as you see, you have a lot of potential undefined behavior, particularily, dereferencing a NULL pointer in your code, you should fix all these things if you want to prevent a SEGMENTATION FAULT.

It's fairly common to see programmers ignore these things, but you should make sure your program wont crash, by just being careful.

• Check for every pointer dereferenced unless it's very evident that it will not be NULL.
• Make every pointer NULL at declaration or right after it, that way you can insure that if it's not pointing to anything yet, then it's NULL, and you can check against that.

Handling pointers is a very difficult thing to get right, but once you develop these good habits, you will never have silly bugs again -> well, almost never!

Answer 2

// always check returned values from system I/O functions
// always enable all warnings so problems are addressed
// do not use 'magic' numbers
// initialize local variables that are pointers
//   (and generally all local variables)
// when an error is encountered during execution,
//    then release allocated memory, close files before exiting
// always look for and handle errors during execution

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <crypt.h> // <-- added for crypt() function

#define MAX_WORD_LEN (200)
#define MAX_STR_LEN  (100)
#define MAX_BUF_LEN  (13)

int main(int argc, char* argv[])
{
    FILE *fd1 = NULL;
    FILE *fd2 = NULL;

    char *str1 = NULL;
    char *str2 = NULL;

    char *salt, *hash, *key, *key1;

    char buf[MAX_BUF_LEN]   = {'\0'};
    char word[MAX_WORD_LEN] = {'\0'};
    char pass[MAX_WORD_LEN] = {'\0'};

    if(argc != 2)
    {
        fprintf(stderr, "Usage: %s <file shadow>\n", argv[0]);
        exit(1);
    }

    // implied else, command line parameter count correct

    if( NULL == (str1 = malloc(MAX_STR_LEN) ))
    { // then, malloc failed
        perror( "malloc for str1 failed" );
        exit( EXIT_FAILURE );
    }

    // implied else, malloc successful

    if( NULL == (str2 = malloc(MAX_STR_LEN) ))
    { // then, malloc failed
        perror( "malloc for str2 failed" );
        free( str1 ); // cleanup
        exit( EXIT_FAILURE );
    }

    // implied else, malloc successful

    if( NULL == (fd1 = fopen(argv[1], "r") ) )
    { // then fopen failed
        perror( "fopen for parameter file name failed" );
        free( str1 ); // cleanup
        free( str2 );
        exit( EXIT_FAILURE );
    }

    // implied else, fopen successful

    fprintf(stderr, "Please, wait...\n");

    while( fgets(str1, MAX_STR_LEN, fd1) )
    {
        if( NULL == (str2 = strstr(str1, "$1$") ) )
        { // then, strstr failed
            perror( "strstr for $1$ failed" );
            continue;
        }

        // implied else, strstr successful

        if( NULL != (key = strtok(str2, ":") ) )
        { // then, strtok failed
            perror( "strtok for : failed" );
            continue;
        }

        // implied else, strtok successful

        snprintf(pass, sizeof(pass), "%s", key);
        printf("pass=%s (%lu)\n", pass, strlen(pass));

        if( NULL == strtok(key, "$") )
        { // then strtok failed
            perror( "strtok for $ failed" );
            continue;
        }

        // implied else, strtok successful

        if( NULL == (salt = strtok(NULL, "$") ) )
        { // then strtok failed
            perror( "strtok for salt failed" );
            continue;
        }

        // implied else, strtok successful

        if( NULL == (hash = strtok(NULL, "\0") ) )
        { // then strtok failed
            perror( "strtok for hash failed" );
            continue;
        }

        // implied else, strtok successful

        snprintf(buf, sizeof(buf), "$1$%s$", salt);

        if( NULL == (fd2 = fopen("polish.txt", "r") ) )
        { // then fopen failed
            perror( "fopen for polish.txt failed" );
            fclose(fd1); // cleanup
            free(str1);
            free(str2);
            exit( EXIT_FAILURE );
        }

        // implied else, fopen successful

        while( fgets(word, MAX_WORD_LEN, fd2) )
        {
            (&word[strlen(word)])[-1] = '\0'; // what last char is being dropped?

            key1 = crypt(word, buf);

            if(!strncmp(key1, pass, strlen(key1)))
            {
                printf("OK!, The password is: %s\n\n", word);
                break;
            }

            else
            {
                printf("Oh No, The password did not match\n\n");
            } // end if
        } // end while

        // prep for next test sequence
        fclose( fd2 );
        fd2 = NULL;
    } // end while

    fclose(fd1);
    fclose(fd2);

    free(str1);
    free(str2);

    return 0;
} // end function: main

Answer 3

The problem is a lack of error checking on this line:

 fd1 = fopen(argv[1], "r");

fopen() returns either a pointer to a populated FILE object, or NULL if it fails to open the file (either the file doesn't exist, or the user in insufficiently privileged to read it).

As a result a NULL pointer is passed into the call to fgets() on line 24.

You should check it for NULL:

fd1 = fopen(argv[1], "r");
if (fd1 == NULL)
{
    fprintf(stderr, "Couldn't open file for reading\n");
    exit(1);
} 

Where we're on the subject of NULL pointers, you should also check the calls to malloc() too. These are less likely to fail, but could also be responsible for the crash on line 24.

Answer 4

C doesn't have exception mechanism like Java. Only way to get information about error it to check value returned from function and sometimes to get more info errno.

In your code you should check if fopen succeed and if not exit with error information.

Your program crashed because you probably try to read file which you don't have read permission and due to fopen return NULL. Hotfix for it is to run program as root or with sudo.

EDIT

I tried it and your program crashed on:

Program received signal SIGSEGV, Segmentation fault. _IO_fgets (buf=0x7fffffffd6a0 "", n=200, fp=0x0) at iofgets.c:50 50 iofgets.c: No such file or directory.

because I don't have polish.txt when I added it, runs smoothly with no errors