Segmentation fault while using strncat in C

Question

So I just started learning two weeks back and am basically a beginner. So I wrote this program. But it continuously gives Segmentation fault error line 41 in the strncat function. I am not sure what I am doing wrong.

#include <stdio.h>
#include <string.h>
#define _XOPEN_SOURCE
#include <unistd.h>

#define _GNU_SOURCE
#include <crypt.h>

#define WORD "/usr/share/dict/words"

int error(void);
int check(char *text, char *psswrd, char *salt);

int main(int argc, char *argv[])
{
    if (argc != 2)
        return error();

    char salt[3]; 
    salt[0] = argv[1][0];
    salt[1] = argv[1][1];
    salt[2] = '\0';

    FILE *fptr;
    char ch;
    char *word;
    int flag = 0;
    fptr = fopen(WORD, "r");
    if (fptr == NULL)
    {
        flag = 1;
    }
    else
    {
        do
        {
            ch = fgetc(fptr);
            if ( ch != ' ' && ch != '\n')
            {
                word = strncat(word, &ch, 1);
                if (strlen(word) == 8)
                {
                    flag = check(word, argv[1], salt);
                    if (flag == 0)
                    {
                        printf ("%s \n", word);
                        return 0;
                    }
                }
            }
            else
            {
                word = "";
            }
        }
        while (ch != EOF);

        fclose(fptr);
    }

    if (flag == 2)
    {
        printf("Password not found\n");
        return 2;
    }
}

int error(void)
{
    printf ("Usage: ./crack <encrypted keyword>\n");
    return 1;
}

int check(char *text, char *psswrd, char *salt)
{
    if (strcmp(crypt(text, salt), psswrd) == 0 )
        return 0;
    else 
        return 2;
 }

For `ch == EOF` to work, `ch` must be an `int`, not a `char`. But if you make it an `int`, you won't be able to use it as an argument to `strncat`. (To my mind, that doesn't matter. `strncat` is a *terrible* way to append a character to a string whose length you know.) — rici, Aug 03 '16 at 17:24
Welcome to Stack Overflow! [Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). — Sourav Ghosh, Aug 03 '16 at 17:35
@SayantanTalukder: `s[i++] = ch; ` is unknown to you? You don't seem to have a problem using that syntax with `salt`. — rici, Aug 03 '16 at 17:55
@rici Actually no. It isn't unknown. I was just being stupid. I was so lot in the trying to add up a string and char ,I totally forgot I can just treat the string as an array of chars. — Sayantan Talukder, Aug 03 '16 at 18:38

score 1 · Answer 1 · edited Aug 03 '16 at 18:49

1

You need to allocate a memory buffer for "word":

word = malloc(MaxInputSize);

Or better would be to just declare word as a static array:

word[MaxInputSize];

edited Aug 03 '16 at 18:49

donjuedo

2,475
18
28

answered Aug 03 '16 at 17:25

Dr.Haimovitz

1,568
12
16

Thanks. The static array worked. (Actually I found the use of array easier to understand) – Sayantan Talukder Aug 03 '16 at 17:42
Don't forget to free it. – Mirakurun Aug 03 '16 at 17:49

sjsam · Answer 2 · 2016-08-03T17:51:20.773

1

on doing

char *word;

you have declared word as a pointer to char but you have not set where word points to. As long as unassigned word can point to anywhere and de-referencing word ie *word causes undefined behavior and that is exactly what you did in

 word = strncat(word, &ch, 1);

For the code to work properly you need to allocate memory to word before the above step ie do something like

word=malloc(100*sizeof *ptr); // You set word to point to a chunk of 100 bytes

Assuming word to point to a chunk of 100 bytes, now

strncat(word, &ch, 1);

wouldn't be logically correct, because assuming word is completely filled( 99 characters plus terminating null character), you can't add one more character to it, so you need to have some error checking prior to this step. Do something like

if strlen(word)<=98)  // remember 99 is the max allowed, you gonna add 1 char
  strncat(word, &ch, 1);

And as a good practice, you could delete the memory after use, ie

free(word);

edited Aug 03 '16 at 17:51

answered Aug 03 '16 at 17:29

sjsam

21,411
5
55
102

You always want to free the memory after you are done. – Mirakurun Aug 03 '16 at 17:49
@Mirakurun : Well,as a good practice, yes, but they will be automatically freed when the program ends anyways – sjsam Aug 03 '16 at 17:54
Not only as a good practice, also for not running out of memory (if the memory does not need to be allocated untill the end of the program) – Dr.Haimovitz Aug 03 '16 at 17:59
@sjsam Don't take memory as granted. – Mirakurun Aug 03 '16 at 18:02

Mark Lakata · Answer 3 · 2017-07-13T21:16:33.013

You need to allocate memory for word, either as an array or using malloc. If the string is small (compared to your stack size), use an array, as it doesn't require cleanup.

You also need to set word[0]=0; after malloc, otherwise you'll get junk and a possible second segmentation fault.

The canonical way of adding characters to a string is

char word[9];
char* pWord = word;
memset(word,0,sizeof(word));
...

  ch = getchar();
  *pWord++ = ch;
  len++;
  if (len >= sizeof(word)-1) ...

Segmentation fault while using strncat in C

3 Answers3