4

I have written a linked list program which stores data member as void *. while trying to store annd print using scanf/printf functions, I am getting segmentation fault.

node definition -->

typedef struct node {
        struct node *next;
        void *data;
        }node;

main function -->

                head=(node *)malloc(sizeof(node));
                if (head==NULL){
                        printf("error in allocation of memory\n");
                        exit(EXIT_FAILURE);
                }
                tail=(node*)create(head);

create function -->

void *create(node *current)
{
        int user_choice;
        while(current){
                printf("\nEnter the data:");
                scanf("%s",current->data);
                printf("stored at %p\n",(void*)current->data);
                printf("%s",(char*)current->data);
                printf("\nType '1' to continue, '0' to exit:\n");
                scanf("%d",&user_choice);

                if(user_choice == 1){
                        current->next=(node*)malloc(sizeof(node));
                        current=current->next;
                }
                else{
                        current->next=NULL;
                }
        }
        return current;
}

can anyone tell what is the correct argument for scanf & prinf should be..?


working code after incorporating points given in answers...

void *create(node *current)
{
        node *temp;
        int user_choice;
        while(current){
                printf("\nEnter the data:");
                current->data=(char*)malloc(10*sizeof(char));
                scanf("%s",current->data);
                printf("stored at %p\n",(void*)current->data);
                printf("%s",(char*)current->data);
                printf("\nType '1' to continue, '0' to exit:\n");
                scanf("%d",&user_choice);

                if(user_choice == 1){
                        current->next=(node*)malloc(sizeof(node));
                }
                else{
                        current->next=NULL;
                        temp=current;
                }
                current=current->next;
        }
        return temp;
}
Himanshu Sourav
  • 700
  • 1
  • 10
  • 35
  • 1
    When the user presses 0, the function will return NULL. There are many errors. Please read a book, rethink your code, and then come back if you have problems. – Paul Ogilvie Dec 07 '16 at 13:13
  • 1
    When `head` is null, and you print the error message (which should go to stderr, not stdout), you should do something other than continue with the next line that requires `head` be non-null. Your program has undefined behaviour. – Toby Speight Dec 07 '16 at 13:33
  • @PaulOgilvie thanks for the sharp observation, I have modified code to rectify this problem and other errors. could you have one more look to see if any other issue is there..? posted in answer. – Himanshu Sourav Dec 08 '16 at 05:30
  • I don't see any improvements. Still many errors. – Paul Ogilvie Dec 08 '16 at 12:29
  • @PaulOgilvie, I had added in answer, updated question as well... – Himanshu Sourav Dec 09 '16 at 04:43

5 Answers5

3

In your code,

 scanf("%s",current->data);

is attempt to make use of an unitialized pointer, it invokes undefined behavior.

You need to follow either of bellow approach,

  • make the pointer point to valid chunk of memory (using malloc() and family for dynamic allocation, for example)
  • use an array.
Sourav Ghosh
  • 133,132
  • 16
  • 183
  • 261
1

You should first initialize data member of structure because

current->data  = malloc("passes size here");

For putting data you have to typecast first this data because void is not storage type. void pointer can be used to point to any data type.

Like

*(char *)(current->data) = 1;
Toby Speight
  • 27,591
  • 48
  • 66
  • 103
Sumit Gemini
  • 1,836
  • 1
  • 15
  • 19
1

As others have said:

scanf("%s",current->data);

Is undefined in C. current->data needs to be pointing somewhere before you can store anything in it.

You should instead:

  1. Accept input from scanf.
  2. Store in temporary buffer.
  3. Insert into linked list
  4. print out whole linked list at the end
  5. free() linked list at the end.

I also feel that your current void *create function is doing too much, and it would be easier to split up your code into different functions, just to make it easier to handle all the pointer operations, inserting etc.

To demonstrate these points, I wrote some code a while ago which does these things, and has been modified to help you with your code. It is not the best code, but it does use these points that will help you with your code.

Here it is:

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

#define MAXSTRLEN 100

typedef struct node {
    void *data;
    struct node *next;
} node_t;

typedef struct {
    node_t *head;
    node_t *foot;
} list_t;

list_t *create_list(void);
node_t *generate_node(void);
list_t *insert_node(list_t *list, char *data);
void print_list(list_t *list);
void free_list(list_t *list);

int
main(int argc, char *argv[]) {
    list_t *list;
    char data[MAXSTRLEN];
    int user_choice;

    list = create_list();

    while (1) {
        printf("Enter the data: ");
        scanf("%s", data);

        printf("\nType '1' to continue, '0' to exit:\n");
        if (scanf("%d",&user_choice) != 1) {
            printf("Invalid input\n");
            exit(EXIT_FAILURE);
        }

        if (user_choice == 1) {
            list = insert_node(list, data);
        } else {
            list = insert_node(list, data);
            break;
        }
    }

    print_list(list);

    free_list(list);
    list = NULL;

    return 0;
}

/* inserting at foot, you can insert at the head if you wish. */
list_t
*insert_node(list_t *list, char *data) {
    node_t *newnode = generate_node();

    newnode->data = malloc(strlen(data)+1);
    strcpy(newnode->data, data);

    newnode->next = NULL;
    if (list->foot == NULL) {
        list->head = newnode;
        list->foot = newnode;
    } else {
        list->foot->next = newnode;
        list->foot = newnode;
    }
    return list;

}

node_t
*generate_node(void) {
    node_t *new = malloc(sizeof(*new));
    new->data = NULL;
    return new;
}

void
print_list(list_t *list) {
    node_t *curr = list->head;

    printf("\nlinked list data:\n");
    while(curr != NULL) {
        printf("%s\n", (char*)curr->data);
        curr = curr->next;
    }
}

list_t
*create_list(void) {
    list_t *list = malloc(sizeof(*list));

    if (list == NULL) {
        fprintf(stderr, "%s\n", "Error allocating memory");
        exit(EXIT_FAILURE);
    }

    list->head = NULL;
    list->foot = NULL;
    return list;
}

void
free_list(list_t *list) {
    node_t *curr, *prev;
    curr = list->head;
    while (curr) {
        prev = curr;
        curr = curr->next;
        free(prev);
    }
    free(list);
}

UPDATE:

Also note how I allocated memory for newnode->data?

Like this:

newnode->data = malloc(strlen(data)+1); //using buffer from scanf

This now means I can store data in this pointer, your current->data will need to do something similar.

RoadRunner
  • 25,803
  • 6
  • 42
  • 75
1

working code-->

void *create(node *current)
{
        node *temp;
        int user_choice;
        while(current){
                printf("\nEnter the data:");
                current->data=(char*)malloc(10*sizeof(char));
                scanf("%s",current->data);
                printf("stored at %p\n",(void*)current->data);
                printf("%s",(char*)current->data);
                printf("\nType '1' to continue, '0' to exit:\n");
                scanf("%d",&user_choice);

                if(user_choice == 1){
                        current->next=(node*)malloc(sizeof(node));
                }
                else{
                        current->next=NULL;
                        temp=current;
                }
                current=current->next;
        }
        return temp;
}
Himanshu Sourav
  • 700
  • 1
  • 10
  • 35
-1

Please try with this

void *create(node *current)
{
        int user_choice;
        while(true){
                if(current == NULL) {
                   current = (node *)malloc(sizeof(node));
                   current->data = NULL;
                   current->next = NULL;
                }
                printf("\nEnter the data:");
                scanf("%s",current->data);
                printf("stored at %p\n", (void *)current->data);
                printf("%s",current->data);
                //printf("%s",(char*)current->data);
                printf("\nType '1' to continue, '0' to exit:\n");
                scanf("%d",&user_choice);

                if(user_choice == 1){
                        current->next=(node*)malloc(sizeof(node));
                        current=current->next;
                }
                else{
                        current->next=NULL;
                        tail = current;
                        current=current->next;
                        break;
                }
        }
        return current;
}

Note: The element has to be initialized (ie; it has to be alloted with some memory) before we are trying to make use of it.

Tom Taylor
  • 3,344
  • 2
  • 38
  • 63
  • You aren't showing that you've allocated any memory for `current->data` at any point. Nor do you always initialize all the elements of a newly allocated node. In C, you always have to think about "where are my pointers pointing". – Jonathan Leffler Dec 07 '16 at 14:31
  • `current = (node *)malloc(sizeof(node));` allocating memory of a node allocates memory for its elements like `current->data` and `current->next` – Tom Taylor Dec 07 '16 at 14:51
  • 2
    But `current->data` will hold a `void*` pointer, which needs to be allocated before use. – RoadRunner Dec 07 '16 at 14:56
  • What I meant is that the data element of the new node doesn't point to anything (it isn't even a null pointer necessarily), but to be able to read data into the space pointed at by the data element, you must first allocate space for it to point at. – Jonathan Leffler Dec 07 '16 at 14:58