Segmentation fault with fgets() receiving input from stdin in C

Question

I'm having a problem with my gdb debugger and every time I try to run the program, the debugger gives out the following error at the line where I use the "fgets"(): _IO_fgets (buf=0x7fffffffe330 "P\343\377\377\377\177", n=2, fp=0x0)

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

typedef struct _node {
   int value ;
   struct _node * next ;
} node ;

void print_avg(node * head, int n)
{
    int sum = 0 , i = 0;
    node * p = head ;
    for (i = 0 ; i < n ; i++) {
            sum += p->value ;
            p = p->next ;
    }
    printf("%f\n", ((float)sum / (float)n)) ;
}

int get_nums(node ** head)
{
    int n = 1 ;
    char line[4]  ;

    while (fgets(line, sizeof(line), stdin) != NULL) {
            //strtok(line, "\n") ;

            node * curr ;
            curr = (node *) malloc(sizeof(node)) ;
            curr->value = atoi(line) ;
            curr->next = *head ;
            *head = curr ;

            n++ ;
    }
    return n ;
}


int main()
{
    int n ;
    node * head = NULL;

    n = get_nums(&head) ;
    print_avg(head, n) ;
    return 0 ;
}

I have no clue what is wrong with my fgets(). Does anybody have an idea?

Answer 1

Your usage of fgets looks correct to me, but there is another problem in your code: n is off by one.

Replace

int n = 1;

by

int n = 0;

because initially the list contains 0 elements, not 1 element.

But anyway this is better:

You should use this this in your for loop.

for (node * p = head; p != NULL; p = p->next)

The list is terminated by a NULL pointer in the next field. So you should check for this condition instead of testing for the number of elements.

Answer 2

using Valgrind I get

Access not within mapped region at address 0x0 at 0x4006D8: print_avg (main.c:16)

The problem is the instruction sum += p->value if p is NULL.

p is NULL because head is NULL because int get_nums(node * head) creates a copy of the pointer to head so when yo do head = curr ;, you're actually storing the value of the pointer into a copy so when you return to main the first head pointer is still NULL and you have a memory leak since you cannot free the memory you allocated (since the pointer is now lost).

here is the real working code you need

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

typedef struct _node {
  int value ;
  struct _node * next ;
 node ;

void print_avg(node ** head, int n)

   int sum = 0 , i = 0;
   node *p = *head ;
   //better check p value here to avoid segmentation fault
   for (i = 0 ; i < n && p!=NULL; i++) {
           sum += p->value ;
           p = (*head)->next ;
           free(*head);
           *head = p;

   }
   printf("%f\n", ((float)sum / (float)n)) ;


int get_nums(node ** head)

   int n = 0 ;
   char line[4]  ;

   while (fgets(line, sizeof(line), stdin)) {
           //strtok(line, "\n") ;

           node * curr ;
           curr = (node *) malloc(sizeof(node)) ;
           curr->value = strtol(line,NULL,10) ;
           curr->next = *head ;
           *head = curr ;

           n++ ;
   }
   return n ;



int main()

   int n ;
   node * head = NULL;
   //here we pass the address of head pointer so that the function will be able to set it
   n = get_nums(&head) ;
   //here we pass the address of head pointer so that the function will be able to free your list of pointers
   print_avg(&head, n) ;
   return 0 ;

More over n must start from 0 in your int get_nums(node ** head) function

Regards!

@Walz In this case you need to pass a pointer to a pointer because otherwise IF you free the nodes inside print function, head pointer from outside the function would not point to the right head of your list after print_avg function. For now let's say I have a pointer A which points to MY_LIST then I copy pointer A inside print_avg to handle MY_LIST, as I free the nodes inside MY_LIST and I return from print_avg ponter A is not changed! So it will point to the previous head (which has been freed) so if I try to access to it via pointer A a segmentation fault just around the corner.

You should always free allocated resources when you are sure you don't need them any more in your program. Since we are accessing the list, the most efficient way to free the list is freeing it node by node as you have used the value to calculate the avg BUT you can always free them later, maybe in another function.

Finally since your main ends there freeing resources is not so important here but it should be like an habit for you to free allocated resources before you loose the pointer to them. Imagine you are writing a multi threaded server and you loose even one byte per connection. Sooner or later you are going out of memory.

Regards!

@MasterGL I don't know if this could be your problem but if I ran the debugger in Eclipse and if I do "step into" when I get to fgets instruction I receive some errors too (maybe the debugger doesn't have fgets source code to show). I did with "Step over" and I only used "Step Into" to enter into my functions. It works well