C - read line from stdin with fgets() in function

Question

I'm trying to read line from stdin with fgets(), I want to use fgets() in my function, which I think is the problem. The string could be max 1024 chars long. When I run this code I get "Segmentation fault (core dumped)"

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

#define MAX_SIZE 1025

void print_fgets();

int main()
{
    print_select();
    return 0;
}

void print_select()
{
    char *str;
    int length;

    while (fgets( str, MAX_SIZE, stdin)!=NULL)
    {
        length=strlen(str);

        if (length==MAX_SIZE-1 && str[length-1]!='\n')
        {
             printf("Error, line overeached buffer!\n");
             return 1;
        }

        if (str[length-1]=='\n')
             str[length-1]='\0';
        printf("%s\n", str);
    }
}

For a fixed-size buffer, one typically allocates `s` using automatic or global storage: `char s[MAX_SIZE];` or `static char s[MAX_SIZE];` if multiple threads aren't an issue. Your length checks are also completely unnecessary. `fgets` guarantees that you won't overrun the buffer. — sfstewman, Nov 03 '14 at 16:23
Note: In rare cases (reading an embedded `'\0'`), `str[0] == 0`, then `str[length-1]` is outside `str`. Suggest `size_t len = strlen(str); if (len > 0 && str[len-1] == '\n') str[--len] = '\0';` — chux - Reinstate Monica, Nov 03 '14 at 16:39

Lucas Godoy · Answer 1 · 2014-11-03T16:38:00.200

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

#define MAX_SIZE 1025

int print_select();  /* Use correct name (instead of print_fgets()) */

int main()
{
    print_select();
    return 0;
}

int print_select()  /* Fix. Dhould return int if you have a return <int> statement. */
{
    char str[MAX_SIZE];  /* Initialize static memory. */
    int length;

    while (fgets( str, MAX_SIZE, stdin)!=NULL)
    {
        length=strlen(str);
        if (length==MAX_SIZE-1 && str[length-1]!='\n')
        {
             printf("Error, line overeached buffer!\n");
             return 1;
        }
        if (str[length-1]=='\n')
        {
             str[length-1]='\0';
        }
        printf("%s\n", str);
    }
    return 0;  /* Function may not be returning an int. Return it in those cases. */
}

There's no need to paste the whole code again. Use the available macro `MAX_SIZE` instead of hardcoded `1025`. — Jite, Nov 03 '14 at 16:30

Jite · Accepted Answer · 2014-11-03T16:31:31.183

The problem is that you try to write to the location that the str pointer points to. Initially it will point to some garbage address (due to char *str not being initialized).

You can try a stack based solution instead by changing:

/* This is just a pointer */
char *str;

to:

/* This is a character array (on the stack) of size MAX_SIZE, initialized to zero */
char str[MAX_SIZE] = {0};

Or if you want to allocate memory dynamically for the array, do this instead:

char *str;
str = calloc(1, MAX_SIZE);
if (str == NULL) {
    /* Failed to allocate memory */
}

... Your code

free(str);  /* You should always free dynamically allocated memory when you are done with it */
str = NULL; /* It's a good practice to point your pointer to NULL after it's free'd */

Don't forget things like array index starting with 0 and goes to MAX_SIZE - 1 (in your case) and NUL termination (strings must end with it).

The initialization is unncessary in this case. `fgets` also *guarantees* NULL termination. — sfstewman, Nov 03 '14 at 16:24
@sfstewman Sure, but I think it's good practice for a beginners to always initialize to zero to avoid problems. Next minute it's not `fgets` thats called but something else that doesn't guarantee null termination for instance. — Jite, Nov 03 '14 at 16:29

C - read line from stdin with fgets() in function

2 Answers2

Linked